A method is too long.
Therefore, take a part of the method that seems useful on its own. (To check this, see if you can find a good name for it.) Turn it into a separate method. Change the original method to use the new one.
Related patterns. If the new method would take a lot of parameters (originally these were local variables or method arguments), try Method Object first. If the code has several validations, see Abortive Validations
Also described on page 110 of Martin Fowler's Refactoring Improving The Design Of Existing Code:
"You have a code fragment that can be grouped together."
"Turn the fragment into a method whose name explains the purpose of the method."
By the way, didn't we have a discussion somewhere about Too Long Methods? Can we hope to reach a kind of agreement about the ideal method length in OO software? -- Marnix Klooster
See It Depends
I wouldn't say that you do this when a method is too long, but when a method does more than one thing, or if it mixes high-level and low-level operations. I know that I've read a pattern by Kent Beck (Composed Method in the Smalltalk Best Practice Patterns) that says not to mix levels within a method.
Therefore, the problem is that a method contains the wrong level of abstraction. The symptom is that the method "feels" too long. I have seen 40 line Smalltalk methods that weren't too long. I also have seen 5 line methods that were.
-- Don Roberts
Can you give an example of a good 40 line Small Talk methods. I have seen such things in C++, and sometimes even Java, because they are not parsimonious languages. But in Small Talk?
-- Aamod Sane
One example is a class initialization method that sets up some table:
initialize SomeClassV''''''ariable := Dictionary new. SomeClassV''''''ariable at: #someValue1 put: #someOtherValue1; ...many more values... at: #someValueN put: #someOtherValueN
-- John Brant
Compare with The Simplest Code. Extract Method seems to go directly against rule 4 (minimizes number of classes and methods). You might argue that it should only be done for facilitating 2 (no duplication) or 3 (expresses all the ideas you want to express), but that doesn't seem to cover all of its uses. Extract Method is clearly a useful and extremely common refactoring technique, but it seems to result in less "simple" code. How can you tell when it's appropriate? Is The Simplest Code what you should write before you start refactoring, or what you want your code to look like all the time? (arguably this discussion would be better placed in Raise Abstraction or Facade Pattern or The Simplest Code or...?) -- An Aspirant
"Expresses all the ideas you want to express" includes the ideas of reducing coupling and increasing cohesion. These are important ideas. (Someone wrote that ALL of the writing about good OO design can be summed up as reduce coupling and increase cohesion.)
Whenever I use Extract Method and Move Method, it is almost always reducing the number of responsibilities of a method or a class (increasing cohesion) and sometimes reducing coupling between two classes. -- Keith Ray
Has anyone taken this to its extreme and always made every block a separate method? (E.g., each multi-statement loop body is extracted into a method, etc.) I can't imagine a reason for applying this refactoring so compulsively (other than perfecting one's talent for giving methods Meaningful Names), but am curious whether anyone has tried it, or practices it regularly.
This is the type of refactoring that I mentioned in Test First And Functional Programming Synergy. To me it's great for making the code more testable. -- David Plumpton
Yes, I do extract every block into its own method. It makes it much clearer as to what is going on and separates program control from program algorithms. When doing a loop, you can concentrate on getting the loop correct and visually verifying the loop. When doing the processing inside the loop, you can concentrate on get the processing correct and visually verifying the processing. -- Wayne Mack
I've tried this, and of course found that I can't keep track of the helper methods. So I put them all in a Method Object, which has been known to reveal a hidden abstraction. The effect of separating looping from processing is so powerful that I tend to give all inner loops their own methods. This can result in amazingly clean code
for(int outer=0;i<array.length;i++) doInnerLoops();
-- David Wright
I think the same principle applies just as well to other control structures such as switches/case statements. I'm currently looking at a state machine with 40+ states implemented in a single 900-line function. (No, I didn't write it.) I'm fairly sure it would be a lot more readable and maintainable if each state consisted of two to three lines of code:
state N: performAppropriatelyNamedAction(); change to state M; break;
The state changes could also be moved out if there were more than one possible target state per action. The state machine would still be one or two hundred lines long for 40+ states, but the control would be a lot easier to debug, and the individual actions would be much better encapsulated and easier to test/debug.
Maybe there's an analogy to the various types of pasta code. This is clearly not Spaghetti Code, or Lasagna Code, or Ravioli Code. Maybe it is Submarine Sandwich Code - goes on for miles with lots of stuff inside.
IMO too many people abuse this principle. I have seen this way too many times: There is a vertically small method (<=1 page). Someone extracts it other method(s), some of which may be in a different part of the solution. Then when debugging, the IDE jumps all over the place, only running 1-2 lines per jump, starts opening all of these other pages (classes) that you have to close later, and it's a nightmare. All the while you are trying to find the section that actually DOES SOMETHING. This is also abused by people who Extract Classes too often and you bounce from base to derived to virtual to interface etc., half of which contain literally no code.
Consider it time to Forget The Debugger perhaps?
Basically many programmers who consider themselves experts have no respect for: debugger jumping, YAGNI abstractions, context switching, or horizontal scrolling (this.that.method.submethod.scrolltillyoudie.etc...)
IMO those aren't forces that deserve respect. In any reasonably large code base, the difference between using 30 line methods and 5 line methods won't significantly change the call stack depth. Debuggers are there to jump for us. YAGNI applies to features, not methods. Horizontal scrolling is decreased by Extract Method, not increased, because each new method shifts code back to the left side of the screen. Closing debugger windows isn't a nightmare in any debugger I've ever used. Most of them will close them all automatically for you, won't they? -- Eric Hodges
That, and most debuggers will allow you to jump over instead of into some methods. So doing this actually gives you choice. You can jump into the lower levels, but you don't have to, choosing to ignore them instead. Having all the code in one method gives you no choice; you have to step through it all. --ATS
See original on c2.com 