Why is a method with too many lines a bad thing? [closed]

Question

In my rails app, 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 flagged this method as Method had too many lines. Why is it bad to write a method with too many lines? What if we have to do a lot of work in it? How can I re-factor this and write good code?

Answer 1

One way is that you can refactor it using 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 are compelled to write a very long method, it means there is something wrong with your Object Oriented Design. Maybe, you need to design a new class for the extra functionality, or you need to split the functionality in the same class by writing multiple methods that when combined, do the job of a single long method.

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

Just like an essay with big paragraphs is harder to read, likewise a program with longer methods is difficult to read, and less likely to re-usable. The more chunks you divide your code in, the more moduler, re-usable and understandable your code will be.

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

If you have to do a lot of work in it; it surely means that you have designed your classes in a way that isn't good. Perhaps, you need to design a new class to handle this functionality, or you need to split up this method into smaller chunks.

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

For that, I highly recommend to you a great book named: Refactoring by Martin Fowler, he is incredibly amazing at explaining how, when, and why to refactor your code.

Answer 2

When I read code with many very short methods I find that it often takes longer than it should to understand how everything fits together. Some of the reasons are illustrated nicely in your example code. Let's try breaking 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 off the bat there are a couple of big problems:

• After reading the method cart I have no idea what it does. That is in part because values are assigned to instance variables rather than returning values to the method that called cart.
• I would like the names of the methods process_current_user and setup_cart to tell the reader what they do. Those names do not do that. They could probably be improved upon, but they were the best I could come up with. The point is that it's not always possible to devise informative method names.

I would like to make all methods other than cart private. That introduces another problem. Do I put all the private methods together at the end--in which case I may have to scroll past several unrelated methods to find them--or do I alternate between public and private methods all through the code? (Of course, this problem can be alleviated somewhat by keeping code files small and using mixins, assuming I can remember which code file does what.)

Consider also what's happened to the number of lines of code. With more lines of code there are more opportunities for mistakes. (I intentionally made a common mistake to illustrate this point. Did you spot it?) It might be easier to test individual methods, but we now need to test that the many separate methods work properly together.

Now let's compare that to your method tweaked just 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 is happening:

• @user is set to current_user if the user is signed in; and
• @cart is assigned to a value that depends on whether the user is signed in and in each case, whether an id is present.

I don't think testing a method of this size is any more difficult than testing my earlier breakdown of the code in smaller methods. In fact, it might be easier to ensure that the tests are comprehensive.

We might also return cart rather than assigning it to an instance variable, and avoid assigning a value to @user if it's only need to determine cart if the user is signed in:

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