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

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

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 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.

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.

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. 😉

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).

Congrats on the new job man

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?

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 😉

Yes there is. 😄

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.

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?

@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.

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 notifications@github.com 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 .

@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…

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

@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.

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?

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. 😀

@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…!

@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.

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. 🙂