Code reviews

[minestrone]

Anyways, there always seems to be some massive desire to break up large methods into small methods for no other reason that people don't like large methods. These people usually have a hard on for Uncle Bob and his dribbling tulipe pile of a book. If a method is a workflow service type arrangement often it is best leaving it in that state.

[d000hg]

The poor sod who has had to fire the out of control new programmer 'Who knows best' but knows nothing.

Yeah, if only they'd use source control.

(that's like a backup system in PM terms)

[woohoo]

Anyways, there always seems to be some massive desire to break up large methods into small methods for no other reason that people don't like large methods. These people usually have a hard on for Uncle Bob and his dribbling tulipe pile of a book. If a method is a workflow service type arrangement often it is best leaving it in that state.

Large methods are usually harder to follow and maintain. If it's doing multiple things then it's harder to re-use the code, refactor and unit test. Often you end up with a tangled mess of code that's difficult to change without breaking something else. You can also end up with lots of repeated code because programmers cant be arsed refactoring the mess of code and just cut and paste.

This is my experience yours might be different, just wanted to say there are lots of good reasons to refactor monolithic classes/methods.

[eek]

Yeah, if only they'd use source control.

(that's like a backup system in PM terms)

Reverting the code back is entirely separate to canning the cleverclogs contractor who annoyed the rest of the team by rewriting the entire code for no good reason....

[Kanye]

Large methods are usually harder to follow and maintain. If it's doing multiple things then it's harder to re-use the code, refactor and unit test. Often you end up with a tangled mess of code that's difficult to change without breaking something else. You can also end up with lots of repeated code because programmers cant be arsed refactoring the mess of code and just cut and paste.

This is my experience yours might be different, just wanted to say there are lots of good reasons to refactor monolithic classes/methods.

Completely agree with that. Small classes and small methods are the way forward.

The first half of this presentation convinced me to almost take this to extremes. Watch around 3 minutes - "methods more than one method long are a code smell" - Aloha Ruby Conf 2012 Refactoring from Good to Great by Ben Orenstein - YouTube

[SpontaneousOrder]

Wow. Thanks for the replies.
There's a whole lot of crazy in this thread. It's possible the scope of change we're talking about is being overestimated (refactoring != redesign). I'm really shocked by some of the responses.

My view was always that you should leave code better than you found it. If you can make simple refactorings such as pulling methods to improve readability, reuse, testablity then its basically your responsibility as a professional to do that.

Exactly. Otherwise you might as well be a code monkey.

Code-rot is a fact of life; and once the rot's set in, if i intertwine new functionality with rotten code I'm just catalysing the whole rotting process.

Comparisons to a decorator doing the wallpaper/ceilings/etc when he was only asked to paint the skirting boards are the wrong analogy. It's more akin to the painter discovering that the skirting boards are peeling badly, so he does a professional job and sands them down first rather than just slapping a new coat on top of the peeling paintwork. The painter isn't interested in the wallpaper or ceilings, and limits the scope of his professionalism to the area which he's been assigned to improve.

Plus the new feature/fix i've implemented is now unit tested, along with whatever i've been able to make testable in the process.

And Tarbera - get a clue! Worst case you see what i've done, say "nice effort, but there are particular risks in this project which you wasn't aware of and it'll be too expensive to redo the performance testing". Then it takes me 3 minutes to revert the code back to it's original state (Oh! not even that because it's not even merged into the release branch because it;s not been approved yet), and you can not pay me for 3 hours of my time, or I can make it up and I'll know better next time.

So a change in a variable name requires same performance testing as breaking a 1000 line program into 15 sub procedures -

that kind of refactoring typically doesn't even change ANY logic. One can go further afterwards if they like, but you can split that huge chunk of code into small methods with descriptive names often without changing any logic at all. You can encapsulate the different responsibilities you discover, in separate classes, again often without changing any logic. Plus I'm aware enough to know whether the code I'm working on is performance critical or not - in fact in this example (in which I did change some logic which the enhancement required) I made the conscious decision to reimplement the new version of the routine in a less efficient way. The trade-off between performance & maintainability was easy to make given the context in which that code is utilised.

Are you one of those excel-monkey (clueless) PMs who has to manufacture importance for themselves?

Anyhow, like i say it's possible some people have overestimated the scale of the refactoring (or just aren't very good at refactoring), so ignore me if that's the case.

Had another batch of reviews today, with a different guy, which went well - infact he started calling me out on the inappropriate multiple responsibilities a particular class was holding, before I pointed out that he was looking at the previous commit, rather than the new one, on the diff. So I might have just got the cantankerous bad egg previously.


I didn't expect a whole load of pages arguing whether it is right or wrong to maintain code as you work with it - it's something I've always taken for granted (at least in my more experienced years after having had a chance to work with the real pros).

I just wanted to see who else was frustrated working with the software dung-beetles who like to live & work in tulip.

[SpontaneousOrder]

Anyways, there always seems to be some massive desire to break up large methods into small methods for no other reason that people don't like large methods. These people usually have a hard on for Uncle Bob and his dribbling tulipe pile of a book. If a method is a workflow service type arrangement often it is best leaving it in that state.

Ok... I like uncle bob (apart from the fact that he's called uncle bob). Not everything he says though, but a lot.

Where I decide to break the methods up is when there are comments in the code, and the comments are necessary. In my experience it is incredibly rare to actually need to put any comments in the code, and often (As in the example i'm posting about) the comments say that the code does something different to what it actually does.

If the code actually NEEDS comments, and a broken-down collection of methods doesn't then need any comments, then do you think thats a strong indicator to do the refactor?

Or are you fine with comments in code (I know lots of people are) ?

I don't like playing the "do i feel lucky, punk?" game when reading comments - so i'd rather just read the code and understand it than having to read the comments, and then read the code as well just to make sure they match.

[minestrone]

Large methods are usually harder to follow and maintain. If it's doing multiple things then it's harder to re-use the code, refactor and unit test. Often you end up with a tangled mess of code that's difficult to change without breaking something else. You can also end up with lots of repeated code because programmers cant be arsed refactoring the mess of code and just cut and paste.

This is my experience yours might be different, just wanted to say there are lots of good reasons to refactor monolithic classes/methods.

Methods should be sized to do a specific task, dogmatically designing your code based on LOC is wrong, as is designing your class for ease of testing. The cleanest code comes from clarity of the interface and the worst thing you can do for that is create methods based on size and methods that make it easier to get your lines of code tested percentage up.

[minestrone]

Wow. Thanks for the replies.
There's a whole lot of crazy in this thread. It's possible the scope of change we're talking about is being overestimated (refactoring != redesign). I'm really shocked by some of the responses.

You changed the classes, created new class signatures and therefore created a new internal API. If you cant see that is a redesign then no wonder the permies are despairing of you.

[SpontaneousOrder]

Methods should be sized to do a specific task, dogmatically designing your code based on LOC is wrong, as is designing your class for ease of testing. The cleanest code comes from clarity of the interface and the worst thing you can do for that is create methods based on size and methods that make it easier to get your lines of code tested percentage up.

I agree with everything you said there, even though I go for small methods. I aim for single responsibility - at both class and method level. I don't think keeping methods small, per se, helps testability, because i factor the large methods out into smaller private methods - and I don't unit test private methods.

Class level single responsibility, though, tends to make testing easier as a side effect. Single reason to change = helpful, easier to test = bonus!

+1 on the not designing code for testability though (apart from special cases).