When does extracting methods from code stop to make sense?

Question

I am currently studying the refactoring methods defined by Marting Fowler (http://refactoring.com/catalog/).

He states a tip for replacing chunks of code by a single method that does that job. So far, I agree, as we all learned about the downsides of Spaghetti-code. But the example for this rule looks as follows:

Replacing

    void printOwing() {
      printBanner();

      //print details
      System.out.println ("name:  " + _name);
      System.out.println ("amount " + getOutstanding());
    }

by

    void printOwing() {
      printBanner();
      printDetails(getOutstanding());
    }

    void printDetails (double outstanding) {
      System.out.println ("name:  " + _name);
      System.out.println ("amount " + outstanding);
    }

Is the readability and thus, the immediate understanding of the code really better in the latter example? There is no indication about what "details" are in reference to "owing". For example, will the name be listed or is there another method printName()? Is the interest listed as well? I would need to search for the printDetail() method's implementation to find out about that.

The method printOwing() itself is already a print-method. Would it not be easier for maintaining the code to just list the System.out.println()'s in this method commenting the purpose as in the first example instead of "scattering" the code this way?

Is there a rule of thumb about when to stop "methodizing" and when it still makes sense?

Answer 1

One thing to understand is that the examples in books are there to illustrate techniques, not necessarily to show the pinnacle of best practice. So if the sweet spot in refactoring was actually to extract 50-line functions from 500-line functions, every example in the book would take several pages and obscure the actual point.

What matters is not necessarily the size of the example, but that you can see and understand the technique used.

In the specific example, the "print details" comment suggests a smell that can be removed by extracting a print_details() function - better to make the functionality explicit and self-documenting rather than rely on comments.

However, unless print_details() had other uses, my personal opinion is that refactoring this would be make-work that takes time away from more important things.

That IS just my opinion : however I'm reading Code Complete at the moment and finding a lot to agree with in it : one point he makes is that studies actually show lower error rates in medium sized functions (50-150 lines) than in either many tiny functions or a few huge functions.

This jives with my own (mainly non-C++) experience though it's at odds with a lot of advice. To see data supporting such large functions is quite a surprise ... "it's not just me then".

Take this too with a pinch of salt :

1. C++ may be different from other languages in optimum function size
2. If your experience suggests a different optimum : 10 or 20 line functions, say, then I wouldn't waste time aggregating them on my or Steve McConnell's say so
3. but I would agree that breaking a 4-line function for the sake of refactoring is unlikely to be worth it (unless each line is very hard to understand!)

Answer 2

Is the readability and thus, the immediate understanding of the code really better in the latter example?

I think it is. By looking at it, you can see that printOwing "prints a banner and some details".

There is no indication about what "details" are in reference to "owing".

If it is not clear or known when reading the API name, then the API should not be called printDetails in the first place (printDebtByName maybe?)

For example, will the name be listed or is there another method printName()?

You should see this by looking at the API or the API docs.

I would need to search for the printDetail() method's implementation to find out about that.

You should actually just choose a better name (than printDetails). This is the reason why naming things is hard in API design. This is also why it is critical. (Stepanov stated in a lecture that for good design to be achieved, you should spend about ten times as much on API design as you spend on the implementation).

The method printOwing() itself is already a print-method. Would it not be easier for maintaining the code to just list the System.out.println()'s in this method commenting the purpose as in the first example instead of "scattering" the code this way?

Not really, though it is easier to write in a single function in the first place (just write everything there, and you're finished). For long-term maintenance though, it is vital that the code is easy to read and understand, not (necessarily) to write.

When doing long term maintenance though, it is easier to refactor if you have such a small change, than it is if you have code that could have been refactored a number of times, but people figured "it's fine where it is" every one of those times (such code becomes bigger and bigger, and then you have to refactor a large method, that would have been naturally broken n times).

The larger the project is, the more critical it is to minimize the comprehension effort.

Is there a rule of thumb about when to stop "methodizing" and when it still makes sense?

The design guide lines that (probably) applied in the decision to refactor:

• keeping similar abstraction levels for similar APIs (both printHeader and printDetails are at a similar abstraction level); printOwed no longer calls APIs with different abstraction levels.

• SRP: printOwned doesn't "print the header and then print the name and then the amount". Instead, it prints "parts of Owned"; Conceptually, it is a different thing.

• Law of Demeter: you minimize the amount of knowledge necessary for each API (printOwned doesn't need to know that something like System.out.println exists at all).