Skip to content

Methods should do one thing - one question #38

@wowremywang

Description

@wowremywang

Hi @uohzxela
Thanks for your good contents. It is really helpful.
But I have a question about Methods should do one thing

Bad:

def email_clients(clients)
  clients.each do |client|
    client_record = database.lookup(client)
    email(client) if client_record.active?
  end
end

email_clients(clients)

Good:

def email_clients(clients)
  clients.each { |client| email(client) }
end

def active_clients(clients)
  clients.select { |client| active_client?(client) }
end

def active_client?(client)
  client_record = database.lookup(client)
  client_record.active?
end

email_clients(active_clients(clients))

It seems Good code is more clear but if we use this code, we have to loop all clients twice.
But first Bad code only loops clients once.
So Bad code is faster than Good code.
What do you think?

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions