Why is a method with too many lines bad?

Question

In my rails application, I have a method like this:

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 is marked with this method as Method had too many lines . Why is it bad to write a method with too many lines? What if we need to work hard? How can I rephrase this and write good code?

Answer 1

One way is that you can reorganize it using the ternary operator , but at the cost of readability.

 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 

Secondly, if you have to write a very long method, it means that something is wrong with the object-oriented design. Perhaps you need to create a new class for additional functionality, or you need to share the functionality in the same class by writing several methods that, when combined, execute one long method.

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

Just as essays with large paragraphs are more difficult to read, similarly, a program with longer methods is difficult to read and less likely to be reused. The more blocks you divide your code, the more modular, reusable and understandable your code will be.

What if we need to work hard?

If you need to work hard at this; this certainly means that you have designed your classes in a way that is not good. You may need to create a new class to handle this function, or you need to break this method into smaller pieces.

How can I rephrase this and write good code?

To do this, I highly recommend you an excellent book called: Martin Fowler's Refactoring , which is incredibly amazing, explaining how, when, and why to reorganize your code.

Answer 2

• Assuming that the number of errors is proportional to the length of the code, it is better to make the code shorter.
• Even if the length of the code is supported (or may be slightly longer), it is better to decompose the long method into short ones, because it is easier for a person to read the short method immediately and find its errors than to read through the long one.

Answer 3

When I read code with many very short methods, I find that it often takes longer than it takes to understand how everything fits together. Some of the reasons are well illustrated in your sample code. Let's try to break it into tiny methods:

 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 

Right after the battle, there are a couple of big problems:

• After reading the cart method, I have no idea what it does. This is partly because values ​​are assigned to instance variables rather than returning values ​​to a method called cart .
• I would like the method names process_current_user and setup_cart tell the reader what they are doing. These names do not do this. They could probably be improved, but they were the best I could think of. The fact is that it is not always possible to create informational method names.

I would like to do all methods other than cart private. This poses another problem. Am I putting all the private methods together at the end - in this case, I may have to scroll through some unrelated methods to find them, or am I alternating between the public and private methods through the whole code? (Of course, this problem can be mitigated a little if you leave the code files small and use mixins, assuming that I can remember which code file does.)

Consider also what happened with the number of lines of code. With more lines of code, there is more room for error. (I intentionally made a general mistake to illustrate this point. Have you noticed this?) It would be easier to test individual methods, but now we need to verify that many individual methods work correctly.

Now compare this with your method, which has changed a bit:

 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 

This tells the reader at a glance what happens:

• @user set to current_user if the user is subscribed; and
• @cart assigned a value depending on whether the user has been signed and in each case whether the identifier is present.

I don’t think that testing a method of this size is more complicated than testing my previous code breakdown into smaller methods. In fact, it would be easier to ensure that the tests are comprehensive.

We can also return cart instead of assigning it to an instance variable and not assigning a @user value if we only need to define cart if the user signed:

 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

You have some excellent answers above, but let me suggest another way to write the same method ...:

 # 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 

As you can see, sometimes if statements are lost. Knowing what your methods return when they fail can make reading and maintaining the write code easier.

As a note, just to understand the || , note that:

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

Good luck

PS

I would think of writing a method in the Cart class for this (or, in your opinion, is this the logic of the 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] )