[yebyen]

I would ask if you're doing Ruby, or object oriented design? Because the first rule of SOLID is Single-Responsibility, and there is this great concept frequently repeated in the OO design circles of Ruby conference talks, "I just want to send a method to an object." I can't say for sure that your method longer than 5 lines is breaking this rule, but if I was a betting man, I'd bet it's breaking one of those rules.

Check out sandi-meter, something much simpler than Rubocop, but also includes this rule about 5 lines method length limit. Maybe you know Sandi Metz, and if you do, maybe you haven't heard of the "Sandi Metz Rules" from POODR; rule number two is "more than 5 lines" – my favorite rule is "controllers with more than one instance variable."

https://github.com/makaroni4/sandi_meter

The great thing about this tool as opposed to RuboCop is that one thing should be really clear when you start using it on an existing code project... the way to interpret the big red spot on your chart is NOT that you should go out and change those things immediately to conform to the new rules.

I understand why you want one instance variable per controller, as the job of a controller is really straightforward, if it only exposes a RESTful interface to a single class of objects, but the more specialized features and bolt-ons it accumulates, the less straightforward it will be to understand and refactor the controller code. But when you're taking an existing controller and adding a new feature to it, the right abstraction to use is probably not obvious until you've spent some time with the idea of the feature, maybe tried out a few different implementations.

For a lot of this stuff, when it clicks, you get it, but before then it seems like these rules have no purpose and it doesn't benefit you to follow them blindly. It pays to know when a rule is important, and when you can safely ignore it.

So, what I would say in response to your question is that simply breaking the method in two is not necessarily the refactor that is suggested by the rules of OO design. You should go back to the principles and try to figure out why that method is longer than 5 lines, and what else about the class has resulted in methods that are so long; is it related to having broken one of the more fundamental rules of design, and is it likely to present an obstacle to later change? (Or is this method never likely to change, and you should just ignore it because... it's fine! This is often the answer.)

Maybe you wrote this method to honor some complicated scheme of ideas, that are really separate ideas, and maybe they should be extracted into separate classes so that it's easier to validate changes to the ideas when it's time to change those business rules. (Or, maybe none of that is true, and they should really be kept in one place because how else are you going to understand all the rules and interactions between them, than by having them together) – Chances are good, though, that the code has broken one of the more fundamental rules, like Open/Closed or Dependency Inversion, and that there is a way to make the method simpler without compromising readability. Maybe there are some heavy calculations that are done inside of the method, and it would be better for DRY to extract them into another method that has a descriptive name, and simply lives nearby.

POODR is a great read I'm told; Refactoring is also an important reference work, and if it's too dry for direct consumption, there are great adaptations that will help get you up to speed on code smells and remediation strategies like https://refactoring.guru

[wyuenho]

I appreciate this detailed response, but I'm afraid if it takes an essay to respond to a simple question of "why break out if more than 5 lines", and the reader with 15 years of programming experience who's well-versed in half a dozen general purpose prog langs still haven't got a clue after reading it, it suggests to me that this is cargo-cult programming.

Ruby is not the first language that has OOPish constructs, but no one else tells devs they are probably writing bad code if they need more than 5 lines of code in a method. I mean, what's wrong with a controller methods that just splits into 2 branches based on the existence of a param, and creates a model and save it to the DB? The entire method will consist of around 20 lines of code. There's no instance variable, no cache and no error handling whatsoever. Do Rubyist get a headache after reading 5?

I'm all for OOP but there seems to be quite some amount of OOP BS around, especially in the Ruby community.

[yebyen]

Is it a method that's longer than 5 lines, or is it a method that is longer than 50? Because one of these things I could see being easily explained away without haranguing, but the other one is what I deal with on my team on a regular basis.

The point I was trying to make isn't that your methods shouldn't be longer than 5 lines, it's that they should be single-responsibility and descriptively named, like the well-designed classes they inhabit.

The 50+ line method which mixes query and command behaviors indiscriminately is the problem. The 5 line method rule is simply one possible distance to the goalposts (and you are free to move the goalpost, no bullshit intended!)

The point is not that methods should be 5 lines or less, full stop. The point is that people who are in the habit of writing methods that are above (threshold value X) ... and leave it this way, boilerplate and all, then spam the same 50 lines everywhere, except on Tuesday when it looks like this other variant... are perhaps contributing to some kind of a trouble state that should trigger at least a second glance whenever your process has you get around to doing code reviews. It's likely one of the same reasons you or your team has reached for this tool to begin with.

If the effect of implementing the tool into your process is that people are still squeezing command and query ideas that don't belong together into the same method, now in fewer lines than ever before, then it's not having all of the desired effect and it sounds like there is still a conversation that needs to happen about what is a good and bad design for a method.

I'm not saying that your personal boilerplate is wrong, (but I am suggesting it might be, based on a metric that is easy to compute.)

It's a tool that you use to identify symptoms of a problem. The symptoms are not the problem, and they may not even be indicative of any real problem. Sometimes you get a sniffle.

[UncleMeat]

> The point is not that methods should be 5 lines or less, full stop.

But that's what a linter does! It just blindly says that shit is wrong. Linters should only emit warnings that people definitely should fix otherwise people stop paying attention and all the useful checks become just noise.

[yebyen]

I'm not saying that this rule is the most beautiful rule, or that you're a bad person if you write methods that are longer than 5 lines and don't get permission from a senior architect first. That's something dysfunctional teams might do.

Look at `--auto-gen-config` because you can totally still use rubocop on codebases that have loads of pre-existing violations in them without fanfare. Then each new violation needs a second look, and an addition in this file. And if the rule is too restrictive, you can change the default away from 5 lines to some bigger number. The principle I hope (Rubocop hopes) you will accept is that short methods are easier to understand than long methods, so shorter methods should be preferred, at least absent other pressures... that's all. The point I guess is to establish a standard, and then iterate on it.

I don't want to start name calling, but Sandi does this talk where she starts out "who knows code smells" and everyone puts their hand up, then she says "who can name 5 code smells" and all the hands go down. Watching this talk was eye opener for me.

Can you write a program that is well organized and doesn't have methods longer than 5 lines? I don't know if I can. Does that mean the metric is bad and should be thrown away without looking back? I'm not ready to go that far, I want to learn more and know how to make this possible, because Sandi and many others with 30+ years experience in OOP have taken time out from their busy day to suggest that it will result in more maintainable code for my team.

There is a big difference between cargo culting and listening to learned experience projected outwardly.

Edit: https://www.youtube.com/watch?v=PJjHfa5yxlU This is the talk about code smells! "Get a Whiff of This" by Sandi Metz, RailsConf 2016

[yebyen]

I will say also that I have >15 years of experience programming in different languages and I've only felt a need to reach for these design concepts in the last 2.

Please don't feel like I'm talking down to you. We are the same. I don't even use these tools, and my methods are very often too long.

[block_dagger]

Ruby is one of the few languages optimized for developer happiness. It consciously makes tradeoffs in favor of that primary purpose. It is natural to expect a focus on the developer experience among its users, such as ensuring code is highly readable (as embodied in the short method reasoning). I do think some developers become overzealous and do sometimes overstate Ruby's features.

[UncleMeat]

I cannot possibly imagine how every method of more than five lines is doing more than one thing.

Heck, I've got plenty of places in my code base where a 100+ line method is absolutely doing a single thing.

[yebyen]

After spending some time to think about your reply, and watching an OO talk or two, I have a clearer way to state why your statement is unlikely to be true in context. I am not an OO "seer" or visionary, I don't mean to say you are broadly wrong, but in the framework of OOD and SOLID, I think that 100+ line method is almost definitely not just doing a single thing.

The Single Responsibility Principle is said to cover all the reasons why a class might need to change. There should be only one reason why a class changes – its single responsibility is that reason. That's for an entire class. Your method lives in the class, so get out your class and read down, method by method and line by line.

Ask yourself periodically while you do this, "can this line of code ever change, in some future state of development?" and "what are the reasons it might change?" – this is not a value judgement, I am just saying that I think by the time you get to the bottom of the method, you'll find the list has decidedly more than one distinct entry in it. (Maybe you don't, and in that case you could have a thing or two to tell me about how you made it that way... please be sure I'm not claiming superiority, especially given that I haven't actually read your code!)

Object Oriented Design is all about managing software change, and making change easy. Your 100+ line method is perhaps not easy to change (and validate.) Maybe it is only called by one other line of code anywhere, and my concerns are ill-founded! It does the same thing every time, and it's just one thing, even though it takes a while and perhaps has many un-named steps.

But if you can think of more than one reason for that method and the class it lives in to need to be changed, then the principles of OOD may say you have run afoul of the S in SOLID, and should maybe reconsider.

One of the things I think we all have a hard time coming to grips with is this maybe reconsider – just because you have identified a code smell, doesn't mean you should fix it! There might be (definitely is) more than one way to fix it, there's also a good chance it might never need fixing.

[yebyen]

Here, this is the talk where the justification and case is made for the "cap your classes at 100 lines" and other 4 rules: https://www.youtube.com/watch?v=npOGOmkxuio

[yebyen]

Your use of the word "every" makes me believe that I have not made my point clearly. I will try to restate it in less than 5 lines so that it can be understood better. (Zing!)

I'm not saying that your code is wrong, (but I am suggesting it might be, based on a metric that is easy to compute.)

It's a tool that you use to identify symptoms of a problem. The symptoms are not the problem.