11

在我的 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. 为什么写一个行数太多的方法不好？如果我们必须在其中做很多工作怎么办？我怎样才能重新考虑这一点并编写好的代码？

4

4 回答 4

9

一种方法是您可以使用三元运算符对其进行重构，但会以可读性为代价。

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

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

为什么写一个行数太多的方法不好？

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

如果我们必须在其中做很多工作怎么办？

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

我怎样才能重新考虑这一点并编写好的代码？

为此，我向您强烈推荐一本名为： Martin Fowler 的《重构》的好书，他在解释如何、何时以及为什么重构您的代码方面非常出色。

于 2015-06-07T03:37:16.927 回答
2

当我使用许多非常短的方法阅读代码时，我发现通常需要比它应该更长的时间来理解所有东西是如何组合在一起的。您的示例代码中很好地说明了一些原因。让我们尝试将其分解为微小的方法：

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我不知道它的作用。这部分是因为值被分配给实例变量，而不是返回值给调用的方法cart。
  • 我想知道方法的名称，process_current_user并setup_cart告诉读者他们做了什么。那些名字不会那样做。它们可能可以改进，但它们是我能想到的最好的。关键是，并非总是可以设计出信息丰富的方法名称。

我想将所有方法设为cart私有以外的所有方法。这就引入了另一个问题。我是否将所有私有方法放在最后——在这种情况下，我可能必须滚动过去几个不相关的方法才能找到它们——或者我是否在整个代码中交替使用公共和私有方法？（当然，如果我能记住哪个代码文件做了什么，这个问题可以通过保持较小的代码文件和使用 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

这一目了然地告诉读者发生了什么：

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

我不认为测试这种规模的方法比用更小的方法测试我之前对代码的分解更困难。事实上，确保测试是全面的可能更容易。

如果只需要确定用户是否登录，我们也可能返回cart而不是将其分配给实例变量，并避免分配值：@usercart

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
于 2015-06-07T06:16:08.443 回答
0
  • 假设错误的数量与代码的长度成正比，最好让代码更短。
  • 即使代码的长度保持不变（或可能稍长），最好将长方法分解为短方法，因为人类一次阅读短方法并发现其错误比阅读长方法更容易一。
于 2015-06-07T03:42:48.767 回答
0

您在上面有很好的答案，但请允许我建议一种不同的方法来编写相同的方法......：

# 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 语句被浪费了。了解您的方法在“失败”时返回的内容可以使编写代码更易于阅读和维护。

作为旁注，为了理解这些||陈述，请注意：

"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 :-)

祝你好运！

附言

我会考虑为此在 Cart 类中编写一个方法（或者您认为这是 Controller 逻辑吗？）：

# 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] )
于 2015-06-07T08:15:42.613 回答