11

私のレールアプリには、次のようなメソッドがあります。

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

1 つの方法は、三項演算子を使用してリファクタリングすることですが、読みやすさが犠牲になります。

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 によるRefactoringという名前の素晴らしい本を強​​くお勧めします。彼は、コードをリファクタリングする方法、時期、理由を非常に優れた方法で説明しています。

于 2015-06-07T03:37:16.927 に答える
0
  • バグの数がコードの長さに比例すると仮定すると、コードは短くした方がよいでしょう。
  • コードの長さが維持されている場合でも (または多少長くなる可能性がある場合でも)、長いメソッドを短いメソッドに分解する方がよいでしょう。長いメソッドを読み通すよりも、短いメソッドを一度に読んでバグを見つける方が人間にとっては簡単だからです。 1。
于 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 :-)

幸運を！

PS

このための 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 に答える