see http://number-none.com/blow/john_carmack_on_inlined_code.htm...
My guideline is basically "as big as it needs to be, without having large pieces of duplicated functionality." If an algorithm requires 100 lines, none of which are repeated or very similar to the others, then so be it.
I wonder what the distribution of function lengths is like in the Linux kernel... there will be longer ones and shorter ones, but a 20-line limit seems absurdly short.
Also vaguely related: http://www.satisfice.com/blog/archives/27
Not only that, but as the OP mentioned that it doesn't work if the methods/functions have side-effects, then we're building up opinions on a sloppy and fundamentally broken house of cards.
First off, your methods should not be having side-effects that are not obvious from the purpose of the method. And then secondly, back to my original point about not having to flow through the code. In light of that, a function should do one thing, and that's it. And when it does so, the method name clearly states what it does. After you put those two together, you simply "assume" that method does what you expect it to, and "flow" through the logic at a higher level.
I'm not going to go down into some silly "read_input_data" method while "flowing" through the high-level code, because I know what it is supposed to do. And by extension, at some point I will review it in its singular/atomic entirety, or would have already done so. Either read_input_data returns the data, or it loads it up into a instance-variable.
Sometimes, but in general I don't agree.
If the functions are made private and giving appropriate names than it's really a way of self-documenting code, which helps.
Suppose you have:
some_function() {
// Calculate interest
// 100 lines of code
// Apply tax
// 120 lines of code
// Store results
// 30 lines of code
}
some_function() {
calculate_interest(...);
apply_tax(...);
store_results(...);
}
Also, there's more restriction in that the single-use code block only has access to the variables passed into it, which makes it easier to parse, since less things could be happening. It serves to prune variables from the local scope.
Say I split a function up into a few sub-functions because it's getting big. Now I have the problem that I'm jumping forwards and backwards through the code when I want to explore what that function does:
SomeMassiveFunction() { SubfunctionA(); SubfunctionB(); SubfunctionC(); SubfunctionD(); }
SubfunctionA() { } ....
In this case, SubfunctionC() might end up a few pages down in source code. So now it's a context switch to go there and then go back.
Now this can be somewhat avoided with good function names (so you don't HAVE to jump backwards and forwards) and keyboard shortcuts (to make the process quicker), but it's still a trade-off that I think needs to be kept in mind.
In the best case you should be able to follow the logic without diving into the other functions, only looking at their implementation details as that particular detail becomes relevant.
It all depends at which stage of development you are at. If the piece of code we're talking about is mature and rarely changing then yes it seems like a reasonable thing to do. If however this section is still under development then I would opt for another way of dealing with readability issue.
I think if what you want is to optimise the readability of the code then a simple comment above each section, combined with block scoping is a good set up for splitting.
As the true nature of the code emerges one can decide to turn the block into local lambdas then later into functions.
Again that's because of the first point I've made here in this comment. If you are still developing the functionality, splitting early means that subsequent reviews&development may miss potential interactions + potential refactorings/simplifications that would have been quite clear if the function was still "messy"
I would love an IDE that showed inline in some sane manner function calls, letting me recourse arbitrarily deep - some thing ala code collapsing and expansion. Extra points if you can show a for chart to side of function calls, class interactions and general code flow, highlighting where your cursor is. More points for a pane that shows what variables have to be in order for a the branch of code you're working in to be reached. A changeable granularity setting would be great for all of these.
I think often times we discuss pros and cons without discussing what they could be if we put some effort into changing the current situation.
If you have to jump around between functions a lot to understand something, the code probably could use some reorganizing.
Often I find the best way to simplify large functions is to tear out sub-blocks and give them name. Loops or large conditional blocks are usually easy to tear out and can usually get very meaningful names. Long stretch of imperative code however does not separate well and probably should remain a very large function.
As someone who uses Resharper's Extract Method a lot this is a nice counterexample to where it's probably inappropriate
I just say though it's very seldom that a huge class/method is justified. Usually there are plenty self-enclosed logic or reusable methods that can be broken out either to utility libraries or a separate class.
My rule of thumb is that you should be able to look at a method/class and understand what it does, and that diminishes quickly over a certain size.
Sure some logic might be in other classes but that's another layer of abstraction
However, in this case it is more about something to keep an eye for for the reviewer -- if the function is large and complicated it may be a good idea to take a closer look at it. Presumably the reviewer won't force the function to be broken into smaller pieces unless it actually improves readability and maintainability of the code.
Now, I feel that it should be the other way. Most of the time you want to keep the code that does something together, splitting it up only to factor out common reusable pieces (that are actually reused).
This is also discussed in the linked article.
I agree with your point. It seems silly to be strict about such things. More so if the codebase is written in Java or any other verbose language.
Sometimes it can be helpful to split up a method to be called once if it helps readability, for example if it helps split up a loop or abstracts the specific way we get a value.