Rubocop: "Method has too many lines" check not smart enough

[jeidsath]

I really like the "Method has too many lines" check, and I think that it helps me write better code. Sometimes its warnings aren't helpful to me, and I would like to to improve the code it flags. But I'd appreciate some discussion before I submit a patch.

Should array initialization count as multiple lines for this check? Ie.,

[['FirstName', first_name],
 ['LastName', last_name],
 ['Email', email],
 ['OrgName', company],
 ['Company', company],
 ['Subdomain__c_lead', ai['subdomain']],
 ['TimeZone', ai['timezone']],
 ['pd_user_id__c', ai['uid']],
 ['pd_test__c', ai['test_mode']],
 ['pd_account_owner_user_id__c', ai['uid']],
 ['Created_At__c_lead', Time.now.strftime('%Y-%m-%d')],
 ['pd_test', test]]

Should operator separated lines count as multiple lines? Ie.,

get_lead_by_marketo_id(client) ||
  get_lead_by_marketo_cookie(client, account_info) ||
  get_lead_by_email(client, account_info) ||
  create_lead_by_email(account_info['email'])

[bbatsov]

Yep, those are valid points. We might benefit from an option called IgnoreMultilineLiterals so solve the first problem. Not sure what we should do (if anything) about the second point you raised.

@yujinakayama @jonas054 @edzhelyov Thoughts?

[jonas054]

A long expression with many operators can add just as much complexity as an equally long series of statements, so I would not like to make any concessions for the second example.

When it comes to long literals, I'm not so sure that an option to disregard them would be good. In the above example, couldn't you let different help methods generate different groups of elements, @jeidsath? I looks like the first few elements are information about a person, for example.

[joseph-ravenwolfe]

Another example of this being an issue - and a very prevalent issue - is strong parameters in Rails. Controllers often define strong parameters with arguments that exceed the maximum number of lines. This is tightly coupled to the number of attributes present in the form. Consequently, almost all of the controllers on a current project raise this offense.

Unfortunately, this is a recommended convention in Rails that is relatively difficult to work around. The following example fails this cop.

class OrdersController < ApplicationController
  def create
    @order = Order.new(order_params)
    # ...
  end

  private

    def order_params
      params.require(:order).permit(
        :ordered_on,
        :name,
        :description,
        :email,
        :phone_number,
        :company,
        :url,
        :shipping_instructions,
        :street1,
        :street2,
        :city,
        :state,
        :zip
      )
    end
end

[yltsrc]

when I faced code like this on my project for the first time, I started looking for solution
and after a while I think value objects is a key to solve this issue
then your code may be looking like this:

class OrdersController < ApplicationController
  def create
    @order = Order.new(OrderParams.new(params))
    # ...
  end
end

[bbatsov]

This issue seems to be going nowhere, so I'm closing it.

[tetherit]

I have this method:

  def create
    @agent = Agent.new(params[:agent])

    respond_to do |format|
      if @agent.save
        notice = t('agents.controller.created')
        format.html { redirect_to agents_path, notice: notice }
        format.json { render json: @agent, status: :created, location: @agent }
      else
        status = :unprocessable_entity
        format.html { render :new }
        format.json { render json: @agent.errors, status: status }
      end
    end
  end

It seems perfectly reasonable to me however Rubocop is throwing the error:

Method has too many lines. [12/10]

Why is this? - Is there some bad code smell in my method I cannot see?

[jonas054]

Yes there is. :smile:

The Ruby Style Guide says "Avoid methods longer than 10 LOC (lines of code). Ideally, most methods will be shorter than 5 LOC. Empty lines do not contribute to the relevant LOC.", and that's why RuboCop has 10 as the _default_ limit.

The subject is not entirely without debate. Some studies indicate that long methods are actually less error-prone than short ones, but you have to go with your conviction some times and not blindly follow the numbers. Some very influential people advocate short methods. Robert C . Martin talks about it in his book Clean Code, Martin Fowler lists Long Method as a code smell in Refactoring, and Sandi Metz sets the limit at 5 in her rules.

[tetherit]

I see, I guess more specifically, how would you write the above to meet these style recommendations?

[jeidsath]

I hate to jump in on what may not be a discussion that will lead anywhere,
but I'd like to share my experience with getting functions down to <10
lines:

1) It often improves my code. But only if I invest time in thinking about
good methods. You MUST pick useful method names. There is no point
otherwise.

2) Sometimes, it does not improve my code. Xanview's example is good. It is
highly symmetric, and therefore easy on eye. Breaking it up is pointless.

3) "if/then/end" is easier on the eye then actual lines of code

4) Instead of short methods, aim for "blinkable" methods. What I mean by
this: Can your brain visually parse the structure of your method at a
glance?

5) In the end, you are going to have to be the judge of your own code. Know
when to reduce the size of your methods and when not to. Use the >10 lines
of code warning as a warning, not an error. Ignore it when appropriate.

On Sat, Apr 12, 2014 at 7:23 AM, xanview [email protected] wrote:

I see, I guess more specifically, how would you write the above to meet
these style recommendations?

—
Reply to this email directly or view it on GitHubhttps://github.com/bbatsov/rubocop/issues/494#issuecomment-40281575
.

[simonc]

We're facing this issue with strong parameters.

@yltsrc your solution may work but it seems overkill to add a new class just to handle a >10LOC method which is perfectly readable, in the right context. I'm aware of the added-value of value-objects but this value is in DRYing the concept, not the code.

Having the choice between lines and statements would be a good thing IMHO ;)

[jonas054]

Look at #1370. The Metrics/AbcSize cop was introduced in response to concerns like this. If you think that methods longer than 10 LOC aren't in themselves a problem, then you raise the MethodLength:Max configuration parameter a bit. If you think that 10 LOC is a good limit and just want to exempt this particular case, then you do it like this:

  def order_params # rubocop:disable Metrics/MethodLength
    # ...
  end

The order_params method has a low score in the AbsSize cop.

[alininja]

@jonas054 Thank you for that tip. Is it possible to disable a rubocop check for a particular method in .rubocop.yml instead of within the source code?

So something like:

Metrics/MethodLength:
  Exclude:
    - 'Klass#method_with_large_hash'

Instead of:

def method_with_large_hash # rubocop:disable Metrics/MethodLength
...
end

I couldn't find anything like it in the documentation so I'm wondering if I just missed it. My motivation is to keep the source code a bit cleaner.

Thanks!

PS: I wasn't sure if creating a new issue was a good place for this question. Sorry, if I'm posting in the wrong place...

[Drenmi]

Hi, @alininja!

There is no such configuration option for this particular cop. The main reason being that a lot of times it is impossible to deduce which class a method call belongs to, and whitelisting just the method name often leads to false negatives.

I can relate to the desire to keep the code "clean", but I've found that having the inline comments helps prevent confusion by explicating (to team mates and future selves) that: "I know this method has a lot of lines, and I'm okay with that." Sometimes even with a supporting comment as to why that's the case. 😀

[bbatsov]

Exactly. @Drenmi is spot on.

[alininja]

@Drenmi, @bbatsov, thanks for clarifying and confirming that the comments are the way to go. Have a great day!

[thg303]

def format_for(row_hash)
    case row_hash[:status]
    when 'ok'
      @normal_format
    when 'pending'
      @pending_format
    when 'rejected'
      @rejected_format
    when 'approved'
      @approved_format
    else
      @exception_format
    end
  end

shouldn't it at least consider methods with a single switch statement?

[jonas054]

@thg303 Are you saying that method is not 12 lines long? Because the MethodLength looks at number of lines. Nothing else. See my comment above.

[thg303]

@jonas054 I mean simply counting method lines is not helpful enough. After all the idea is to help human understands the code. It could be more helpful if this metric considers the content too. I mean not as deep as the AbcSize does, but at least considers if lines are presenting a single switch or a long hash, showing warning there does not help.

[jonas054]

I guess we'll have to agree to disagree on this. :smile: It feels to me like you're looking for a different metric. It's no longer method length, and not ABC size either, but something like "perceived size" (we already have Metrics/PerceivedComplexity).

[flajann2]

I raise a bigger question: What really constitutes "better code"? We all have various coding styles and the like. Rubocop even bitches about "too many lines" in Gemfile blocks!!!! That makes no sense at all, and I had to split up a development block into two just to keep it happy.

Just for fun, I ran Rubocop on some of my -- non-Rails -- code, and it literally came back with hundreds of complaints. One project came back with over 600, and it's not a big one either.

I am a pragmatic individual, and I ask the question: What does it buy me to make all of my code Rubocop-happy? It would take a considerable amount of time, and I risk introducing errors in the process -- at something no one is really going to care about anyway.

I'm a long term software developer -- over 36 years. I started out with Basic, but quickly progressed to C and Assembly. In those days, C had lint, and Rubocop is basically lint for Ruby. But in my humble opinion, it seems too picky, complaining about not using parens around function arguments, or using a "superfluous" return, or even not freezing objects assigned to constants (which I can see a good argument for, but still).

[backus]

@flajann2 RuboCop defaults to some community styles that many people like but it is incredibly configurable. You can turn off everything to don't like and you can even just run rubocop --lint if you don't like the style stuff.

[Drenmi]

I raise a bigger question: What really constitutes "better code"?

This is a question we should all ask ourselves, all the time. To me, "better" code means easier to change, which in turn entails things like being readable, understandable, and testable. RuboCop can't tell if something is readable and understandable (because it neither reads nor understands code), but it can provide some proxy measures, albeit primitive.

Another factor is consistency. Good code is consistent, no matter what style was settled on, and RuboCop helps us with that, because even if we're senior professionals, it's impossible to keep hundreds of style rules in mind at once, and even if we could, we'll always make small mistakes. 🙂

We all have various coding styles and the like.

Yes. We acknowledge that, and RuboCop has changed focus from enforcing the Style Guide to being completely configurable. 🙂

I had to split up a development block into two just to keep it happy.

This is quite a common mistake. RuboCop isn't very smart, testament to the fact that static code analysis in a dynamic language is hard. We must always ask "is this reasonable?", and in the case of Gemfile blocks, it doesn't, so we should make the Metrics/BlockLength cop ignore the Gemfile.

Just for fun, I ran Rubocop on some of my -- non-Rails -- code, and it literally came back with hundreds of complaints. One project came back with over 600, and it's not a big one either.

Introducing RuboCop in an existing project is definitely much harder than using it from the get-go. In these cases, most of the offenses tend to come from a small number of cops, so it could be a good point in time to decide whether to change the configured style, or maybe even disable them.

Other useful tools when working with legacy code are the flags --auto-gen-config, which gives you a TODO list and temporarily silences existing offenses, and --auto-correct.

I am a pragmatic individual, and I ask the question: What does it buy me to make all of my code Rubocop-happy? It would take a considerable amount of time, and I risk introducing errors in the process -- at something no one is really going to care about anyway.

I already mentioned consistency. Another benefit is that programmatic enforcement is cheaper than education by at least an order of magnitude, so it helps onboard new developers onto your project, and to make them better Rubyists. If you do code reviews, not having to nit-pick, and being able to focus on the bigger picture is very valuable in my experience. In addition, it helps interpersonal relations if the nit-picking is done by a machine. (Although you're always welcome to vent your frustrations here. 😬)

Also, you not caring is not the same as no-one caring. 😉 RuboCop has 15 million downloads, and thousands of development hours, so clearly people care quite a bit about maintaining a consistent style in their projects.

I'm a long term software developer -- over 36 years. I started out with Basic, but quickly progressed to C and Assembly. In those days, C had lint, and Rubocop is basically lint for Ruby. But in my humble opinion, it seems too picky, complaining about not using parens around function arguments, or using a "superfluous" return, or even not freezing objects assigned to constants (which I can see a good argument for, but still).

I think it is fair for everyone to decide how picky they want to be with their code. There are a lot of cops, so it can take a while to get all the configuration the way you want it, but once you've gone through the process, you have a .rubocop.yml that you can just copy over to new projects and tweak as you go along. 🙂

RuboCop is far from perfect, fraught with limitations and difficult trade-offs. It is not an AI, and it can't write "better code" on its own--and we should be glad that it can't, because if application programming was that deterministic, we might all be out of jobs already. 😉

[flajann2]

Thanks for your reactions. Rubocop is useful in catching some dangerous uses, as it caught the use of eval that can be dangerous especially eval'ing "code" coming in from an insecure channel (think a cheap way to parse JSON coming in from MQTT!!!)

I've gotten over the "shock to the system" Rubocop initially caused me (!). Fellow developers I am working with claim it helps them write better code. To me, it's still like training wheels on a bicycle. But it has its uses.

I like how it integrates well with Emacs. With just 3 keystrokes, I can cop an entire project, and just click the complaints to jump straight to the code. I imagine other IDEs have something similar.

There are some things that still drives me insane about Rubocop -- it's treatment of the curly braces. It really doesn't like them for blocks, and I allowed it to "autocorrect" some code which made the code much uglier as a result.

Yep, analyzing dynamic code with a static analyser is hard. Worse, parsing Ruby is hard.

To me, just using the --lint option is enough, but I have to work with other developers that swears by Rubocop. I will be instituting some changes. For now, I am turning off a lot with rubocop:disable in the code itself, which we'll grep for and talk about reconfiguring it in general.

[brycesenz]

Reading over this thread was incredibly useful, since our team originally had the same complaint. After reflection, I agree with @jonas054 that it makes the most sense to just blindly check the line length. We've had success with the practice of just sanity checking the times when we all agree that this rule should be violated, and using # rubocop:disable MethodLength above the method name as a way to denote that our team has agreed with that decision.

I just wanted to share in case anyone else would benefit.

[flajann2]

Though I must say that Rubocop takes all the fun out of writing code. I am not talking about writing bad code. I am talking about writing good code in our individual styles. I've been writing code for nearly 40 years, and I don't need a stupid drone dictating to me how to structure my blocks, say. I have reasons I may want to use the curly braces in one place, and do-end blocks in another. It's all about esthetics.

Well, I am moving away from Ruby anyway. I now use Elixir. And Rust. And Haskell. And will enjoy them while the fun is still there.

[mwoodiupui]

@flajann2, you need to negotiate with the rest of your dev. team, agree on settings everyone can live with, and get back to writing code. RuboCop is a tool, and we are its masters. Used well, such tools are like having an inexhaustible mentor always at your side. But just adopting a tool with its default settings may set you up for a culture clash between your shop and the one that produced the tool.

I frequently disagree with Ruby community standards, but I have the controls to override them so long as the team accepts my suggestions. They're usually reasonable.

[flajann2]

@mwoodiupui and everyone else, including those who gave me the many thumbs-down: Thank you all for your input. At this juncture, I've gone back to C++, something I haven't done much with in almost 20 years. My career began with C & C++ (and note I do NOT do the inane "C/C++", as they are hugely different!) I like the sheer power of the new C++17. It's almost a new language from what I remember.

The Ruby culture is evolving into something I don't like. Not just the style-Nazification and the super-short methods. Maybe I am simply bored with Ruby itself, as it is very slow for my growing needs. I need computational power, and that is what C++ is for -- and Rust, Haskell, and some other compiled languages. The LLVM is damned awesome, basically a cross-compiler out of the box, which Clang and Rust makes use of -- and so will I on this new Neural Net project I'm working on.

Ruby is used mostly by startups that are under-budget and are more about pushing something quickly out of the door than spending time crafting good, solid, robust code. And the web stuff is just boring out-right. You'll not see too many games written in Ruby. Support for doing stand-alone GUI applications with Ruby is abysmal -- and I've done a few things to enhance this, but still.

Web stuff and scripting is what Ruby is best at, but not much innovation there. And I've implemented several complex algorithms in Ruby, and keeping the methods to less than 10 lines? Don't make me laugh. It would actually make the algorithm harder to understand, as you would not be able to just read it in a linear fashion. Your eyes would have to jump around to all the tiny methods to see what each one does, and then try to pull it all together to understand the entire thing.

I have battled with some over the 10-line issue, and one guy insists that even 10 lines is too long. Ye cats.

Well, no one language is good for everything. Have fun with your Ruby. I gave it a whirl for 10 years, and I am not happy. So C++ it is, where solid companies that can afford my rates use, and the emphasis is on producing good, solid, reliable code, not push something out of the door yesterday.

And I will almost swear that you will find zero cars running Ruby in their embedded systems. (Yes, I am now in the Automotive Industry).

I will continue to maintain my Gems and keep them functional with the newest releases of Ruby, and may even create Ruby bindings for my AI work. But I don't think I'll be doing any more Ruby gigs anytime soon.

[backus]

Congrats on the new job man

[flajann2]

@backus Thanks. I thought it would take me a while to get back into the C++ market, but it happened quickly. I had a lot of catching up to do, as today's C++ is not your grandfather's C++ anymore! Ah, but the wonders of the Internet. There is tons of high-quality resources available.

[Drenmi]

I'm sorry it took so long to figure out that your strong interest is actually in technology, and not in product. I sincerely think there are many developers out there who are quite unhappy working on product (which, outside of Japan, is what Ruby is used for), without knowing why.

Enjoy the C++ and embedded programming. It sounds like an exciting place to be. 🙂

[flajann2]

@Drenmi I have tried many languages -- Java, Python, Rust, Haskell, Perl... Ruby as a language is OK for the problem space it's suited for. I still would like to bring Ruby into the realm of machine learning, but it will be tough to unseat Python from that position.

And yes, I am much more interested in the technology than the product. Most products, alas, fail, or have a limited lifespan. I doubt seriously if any code that I wrote even 10 years ago is still in production today.

I am also intrigued by complexity, and C++ definitely has that. Ruby makes things almost too easy. Though, metaprogramming in Ruby is sweet, and I have done a lot with it. Metaprogramming in C++ is a completely different universe. The power of he dark side...!