Skip to content

Methods should do one thing - one question #38

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?

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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