Extract Method

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 MethodObject first. If the code has several validations, see AbortiveValidations

Also described on page 110 of MartinFowler's RefactoringImprovingTheDesignOfExistingCode:

: "You have a code fragment that can be grouped together."
: "Turn the fragment into a method whose name explains the purpose of the method."

http://martinfowler.com/refactoring/catalog/extractMethod.html

By the way, didn't we have a discussion somewhere about TooLongMethods?? Can we hope to reach a kind of agreement about the ideal method length in OO software? -- MarnixKlooster

See ItDepends

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 KentBeck (ComposedMethod in the SmalltalkBestPracticePatterns) 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. -- DonRoberts

Can you give an example of a good 40 line SmallTalk methods. I have seen such things in C++, and sometimes even Java, because they are not parsimonious languages. But in SmallTalk? -- AamodSane

One example is a class initialization method that sets up some table:

initialize SomeClassVariable := Dictionary new. SomeClassVariable at: #someValue1 put: #someOtherValue1; ...many more values... at: #someValueN put: #someOtherValueN

-- JohnBrant

Compare with TheSimplestCode. ExtractMethod 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. ExtractMethod 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 TheSimplestCode 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 RaiseAbstraction or FacadePattern or TheSimplestCode or...?) -- AnAspirant

"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 ExtractMethod and MoveMethod, it is almost always reducing the number of responsibilities of a method or a class (increasing cohesion) and sometimes reducing coupling between two classes. -- KeithRay

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 MeaningfulNames), but am curious whether anyone has tried it, or practices it regularly.

This is the type of refactoring that I mentioned in TestFirstAndFunctionalProgrammingSynergy. To me it's great for making the code more testable. -- DavidPlumpton

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

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 MethodObject, 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();

-- DavidWright

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 SpaghettiCode, or LasagnaCode, or RavioliCode. Maybe it is SubmarineSandwichCode? - 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 ExtractClasses? 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 ForgetTheDebugger 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 ExtractMethod, 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? -- EricHodges

: 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 MethodsVsCodeFragments, RaiseAbstraction

CategoryRefactoring