uohzxela / clean-code-ruby Goto Github PK

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?

Hi,

I really like this repo. I can see it being immensely useful when working on PR reviews in my company. Instead of explaining something I can simply link to a section in this repo.

Similarly to eg ruby-style-guide what do you think about creating a standalone github organization for this repo? The full link to this repo would be something like: https://github.com/clean-code-ruby/clean-code-ruby. I think this would be really nice and give this repo a nice, "official touch".

https://github.com/uohzxela/clean-code-ruby#use-default-arguments-instead-of-short-circuiting-or-conditionals

The empty strings are not falsy in Ruby

"However, you have to be aware that in some situations, using attr_accessor is a code smell, read more here." Check this link, ASAP!

DRY is more prevailing than removing duplicate code.

rubocop/ruby-style-guide#423

I noticed there was no description under Function names should say what they do. I'd like to suggest something like the following:

### Function names should say what they do

Poorly named methods add to the code reviewer's cognitive load at best, and mislead the
code reviewer at worst. Strive to capture the the precise intent when naming methods.

Hi,

Under the "Test" section you write : "There's no excuse to not write tests. Ruby comes with its own testing tool (RSpec) built right in.". I guess you meant Minitest ?

Great work, thanks !

Cheers.

I think the local variable in the method split_into_first_and_last_name is different from the one name = 'Ryan McDermott' . So it will not affect the outside name variable's value.

Method names should say what they do
Poorly named methods add to the code reviewer's cognitive load at best, and mislead the code reviewer at worst. Strive to capture** the the**precise intent when naming methods.

In ruby:

a = nil || 'a'   
=> 'a'

a = false || 'a'
=> 'a'

The function arguments section talks about rarely using over two arguments in a single method. We should probably give an example that doesn't use four arguments.

Bad:

-def create_menu(title, body, button_text, cancellable)
+def create_menu(title, body)
  # ...
end

Good:

-def create_menu(title:, body:, button_text:, cancellable:)
+def create_menu(title:, body:)
  # ...
end

-create_menu(
-  title: 'Foo',
-  body: 'Bar',
-  button_text: 'Baz',
-  cancellable: true
-)
+create_menu(title: 'Foo', body: 'Bar')

Would be happy to push up a PR for the above :)

I've seen that you have used Functions as a term in several places, that's not really correct.

In Ruby it isn't possible to execute a piece of code that isn't defined on an object, because there is nothing in Ruby that is not an object - this way, strictly speaking, a concept of a function does not exist, there are only methods.

Many of the code samples are not idiomatic. We should use RuboCop to make them conform to existing community-driven style.

I don't disagree with the sentiment here, but Favor functional programming over imperative programming states that "Functional languages are cleaner and easier to test".

The test for the example doesn't change between implementations, so doesn't give the reader an indication of why functional style is easier to test.

require 'minitest/autorun'

class FunctionalVsImperativeTest < Minitest::Test
  def test_imperative
    calculator = Imperative.new
    assert_equal(calculator.calculate(test_data), 3150)
  end

  def test_functional
    calculator = Functional.new
    assert_equal(calculator.calculate(test_data), 3150)
  end

  private

  def test_data
    [ { name: 'Uncle Bobby',
        lines_of_code: 500 },
      { name: 'Suzie Q',
        lines_of_code: 1500 },
      { name: 'Jimmy Gosling',
        lines_of_code: 150 },
      { name: 'Grace Hopper',
        lines_of_code: 1000 } ]
  end
end

class Imperative
  def calculate(programmer_output)
    total_output = 0

    programmer_output.each do |output|
      total_output += output[:lines_of_code]
    end

    total_output
  end
end

class Functional
  INITIAL_VALUE = 0

  def calculate(programmer_output)
    programmer_output.sum(INITIAL_VALUE) { |output| output[:lines_of_code] }
  end
end