为什么行数太多的方法是一件坏事？

Question

在我的 rails 应用程序中，我有一个这样的方法：

def cart
    if user_signed_in?
        @user = current_user
        if @user.cart.present?
            @cart = @user.cart
        else
            @cart = false
        end
    else
        cart_id = session[:cart_id]

        if cart_id.present?
            @cart = Cart.find(cart_id.to_i)
        else
            @cart = false
        end
    end
end

Rubocop 将此方法标记为 Method had too many lines。为什么写太多行的方法不好？如果我们必须在其中做很多工作怎么办？我怎样才能重构它并编写好的代码？

Answer 1

一种方法是您可以使用 ternary operator 重构它，但以牺牲可读性为代价。

def cart
  if user_signed_in?
    @user = current_user
    @cart = @user.cart.present? ? @user.cart : false
  else
    cart_id = session[:cart_id]
    @cart = cart_id.present? ? Cart.find(cart_id.to_i) : false
  end
end

其次，如果你被迫写一个很长的方法，这意味着你的面向对象设计有问题。也许，您需要为额外的功能设计一个新的 class，或者您需要通过编写多个方法来拆分同一个 class 中的功能，这些方法在组合时可以完成单个长方法的工作。

Why is it bad to write a method with too many lines?

就像大段落的文章更难阅读一样，方法较长的程序也很难阅读，而且不太可能重用。您将代码分成的块越多，您的代码就越模块化、可重用和可理解。

What if we have to do a lot of work in it?

如果你必须在其中做很多工作；这肯定意味着您以不好的方式设计了 classes。也许，您需要设计一个新的 class 来处理这个功能，或者您需要将这个方法拆分成更小的块。

How can I re-factor this and write good code?

为此，我强烈向您推荐一本名为：Refactoring 的好书，作者 Martin Fowler，他在解释重构代码的方式、时间和原因方面令人难以置信。

Answer 2

假设错误的数量与代码的长度成正比，最好让代码更短。
即使代码的长度保持不变（或者可能稍微变长），最好将一个长的方法分解成多个短的方法，因为短的方法比人更容易一次阅读并找到它的错误。通读一篇长文。

Answer 3

当我阅读包含许多非常简短的方法的代码时，我发现理解所有内容如何组合在一起所花费的时间通常比应有的时间长。您的示例代码中很好地说明了一些原因。让我们试着把它分解成小方法：

def cart
  if user_signed_in?
    process_current_user
  else
    setup_cart
  end
end

private

def process_current_user
  @user = current_user
  if @user.cart.present?
    assign_cart_when_user_present
  else
    assign_cart_when_user_not_present
  end
end

def assign_cart_when_user_present
  @cart = @user.cart
end

def assign_cart_when_user_not_present
  @cart = false
end

def setup_cart
  cart_id = session[:cart_id]
  if cart_id.present?
    assign_cart_when_id_present
  else
    assign_cart_when_id_present
  end
end

def assign_cart_when_id_present
  @cart = Cart.find(cart_id.to_i)
end

def assign_cart_when_id_not_present
  @cart = false
end

马上就有几个大问题：

看完方法后cart我不知道它有什么作用。这部分是因为值被分配给实例变量，而不是 return 将值分配给调用 cart.
我希望方法 process_current_user 和 setup_cart 的名称能够告诉 reader 它们的作用。那些名字不会那样做。它们可能可以改进，但它们是我能想到的最好的。关键是设计信息丰富的方法名称并不总是可能的。

我想将 cart 以外的所有方法设为私有。这就引入了另一个问题。我是在最后将所有私有方法放在一起——在这种情况下，我可能必须滚动浏览几个不相关的方法才能找到它们——还是在整个代码中交替使用 public 和私有方法？（当然，这个问题可以通过保持代码文件小和使用 mixins 来缓解，假设我能记住哪个代码文件做了什么。）

还要考虑代码行数的变化。代码行越多，出错的机会就越多。（我故意犯了一个常见错误来说明这一点。你发现了吗？）测试单个方法可能更容易，但我们现在需要测试许多单独的方法是否可以一起正常工作。

现在让我们将其与稍作调整的方法进行比较：

def cart
  if user_signed_in?
    @user = current_user
    @cart = case @user.cart.present?
            when true then @user.cart
            else           false
            end
  else
    cart_id = session[:cart_id]
    @cart = case cart_id.present?
       when true then Cart.find(cart_id.to_i)
       else           false
    end
  end
end

这让 reader 一目了然地知道发生了什么：

@user 设置为 current_user；和
@cart 分配给一个值，该值取决于用户是否登录以及在每种情况下是否存在 ID。

我认为测试这种大小的方法并不比在更小的方法中测试我之前的代码分解更困难。事实上，确保测试全面可能更容易。

我们也可能return cart 而不是将其赋值给一个实例变量，并且避免给@user赋值，如果它只需要确定cart 如果用户已登录：

def cart
  if user_signed_in?
    cart = current_user.cart        
    case cart.present?
    when true then cart
    else           false
    end
  else
    cart_id = session[:cart_id]
    case cart_id.present?
    when true then Cart.find(cart_id.to_i)
    else           false
    end
  end
end

Answer 4

你上面的答案很好，但请允许我建议用不同的方式来编写相同的方法...:[=14=]

# by the way, I would change the name of the method, as cart isn't a property of the controller.
def cart
    # Assume that `current_user` returns nil or false if a user isn't logged in.
    @user = current_user

    # The following || operator replaces the if-else statements.
    # If both fail, @cart will be nil or false.
    @cart = (@user && @user.cart) || ( session[:cart_id] && Cart.find(session[:cart_id]) )
end

如您所见，有时 if 语句是浪费的。在 "fail" 时了解您的方法 return 可以使编写的代码更易于阅读和维护。

作为旁注，为了理解 || 语句，请注意：

"anything" && 8 # => 8 # if the statements is true, the second value is returned
"anything" && nil && 5 # => nil # The statement stopped evaluating on `nil`.
# hence, if @user and @user.cart exist:
@user && @user.cart #=> @user.cart # ... Presto :-)

祝你好运！

P.S.

我会考虑为此在购物车 class 中编写一个方法（或者您认为这是控制器逻辑吗？）：

# in cart.rb
class Cart
   def self.find_active user, cart_id
      return user.cart if user
      return self.find(cart_id) if cart_id
      false
   end
end

# in controller:

@cart = Cart.find_active( (@user = current_user), session[:cart_id] )

为什么行数太多的方法是一件坏事？

Why is a method with too many lines a bad thing?

ruby

coding-style

rubocop