2

I was just working with a method of about 70 lines of code developed by others. It uses a very nice pattern structure, stuff like IOC container, but I wonder.. The method is too long?

Is it a so long method still easily readable?

The question could be finished, but now starts the premise from which my question has originated.

Before realizing its length (excessive for me) I was getting bored while trying to understand what it was doing.

It's full of instruction of this kind:

if (success == null)
{
    //Log response error
    logger.LogProviderActionResponse(userId, "actionName", transactionRequestId,
        _configuration.ResponseLogErrorTryAgainId, xmlResponse.InnerXml.ToXml(true));
    return new CheckActionResult(ActionStatus.Failed);
}

What is it doing and why? It deals with logging of course, because the success variable is null, but what does it imply? And also there are many parameters, what is their meaning?

I would need to read it carefully, to study it.

The problem is that this method implements an interface and an abstract class, it has many siblings. Copy and pasted siblings which share the same structural behavior. But it's not immediate to distinguish structural information from domain specific information.

In developing a new action or a new provider I should only need to copy, paste and modify domain specific stuff. But it's hard.. since they are deeply mixed together with structural behavior...

Refactoring I would do to raise the readability

  • 1st step

    surround all this logging with a #region that can be collapsed

  • 2nd step could be

    to use Extract Method to trasnsform the previous log call to a single instruction. Something like:

    Log.LogWithReason(Success.IsEmpty);

But the usage of return conflicts with this purpose requiring extra effort.

  • 3rd step

    would be to completely separate the Logging responsability (using AOP? Implementing it in the base class? using events?). If one method has 5 / 8 calls to the log for different reasons. It's too much aware of logging. In my opinion the Single responsibility principle is violated since the methods has gained the secondary responsability of being strongly aware about logging.

The question remains the one specified in the title but I hope to read a very mature comparison here based on the seeds idea I've wrote!

Thanks for your contribute

M.F05051985
  • 193
  • 5
  • 2
    Wrong. 1st step : Write some unit tests. – Euphoric Apr 11 '14 at 09:37
  • @Euphoric: that is something one should do regardless of step 1, 2 or 3, so it does not fit well into the above list of refactoring alternatives. – Doc Brown Apr 11 '14 at 10:26
  • @DocBrown: which do you think should be the code coverage percentage? – M.F05051985 Apr 11 '14 at 10:55
  • 1
    @M.F05051985 impossible to tell, and different tools use different methods to calculate the number. E.g. one may count comment lines as uncovered (I've seen this), another lines with only white space as uncovered (I've seen that too), a third ignores all of those, yet another I've encountered counts lines with field definitions as uncovered (when there's no way to test those), etc. etc. – jwenting Apr 11 '14 at 11:03
  • 1
    possible duplicate of [What is the ideal length of a method?](http://programmers.stackexchange.com/questions/133404/what-is-the-ideal-length-of-a-method) – Blrfl Apr 11 '14 at 11:38
  • @jwenting: interesting. However do you know [Fermi's questions][1]? If you should give an answer in any way what would you say? [1]http://en.wikipedia.org/wiki/Fermi_problem – M.F05051985 Apr 11 '14 at 11:58
  • @M.F05051985 you're assuming that there's a single fixed answer "x lines are too much, x-1 is good" even possible by approximation. I don't make that assumption. Same with code coverage. If 99% of your code is autogenerated, for example, you don't have to touch 99% of your code as long as the generation tools are trusted to produce correct results. If your organisation decides that single line getters and setters need no unit tests, they need not be covered (though they likely will be covered implicitly through other calls). – jwenting Apr 11 '14 at 12:07
  • @Bart van Ingen Schenau, jwenting, gnat, Blrfl, MichaelT: i think the question is not **primarly** opinion based.. since it's possible to provide an answer based on Evidence Based Software Engineering for example. And also I wonder.. if this question is primarly opinion based what makes every pattern, best practice and so on different? Everything can be based on opinion or on evidence. Evidence searches are really scientific papers only. – M.F05051985 Apr 11 '14 at 14:44
  • @jwenting: thanks! however have a look to Fermi's question on wikipedia.. They are often used also during interview, the logic behind is that an answer could also contains indications, examples, etc. Your answer add some information gain, but not in the direction of answering the question. Saying that in your experience you had been working with 30% coverage or with 3% of with 99% would result in very different feedbacks. – M.F05051985 Apr 11 '14 at 14:49

2 Answers2

3

@jwenting is right that the number of lines should not be the primary reason for refactoring methods. Nevertheless it is a often an indicator for a "code smell". And when the situation is so clear as in your logging example, where the SRP is clearly violated, the method should be refactored. So your main criteria for "when to refactor" should be the SRP itself, the other SOLID principles, and the "single level of abstraction" (SLA) principle.

And the "number of lines" of a method should only be used as an indicator for "which method to check if it violates those principle and can be improved".

As you asked about "software engineering": if you are going to write something like a style guide for your code, don't add a rule like "avoid methods longer than XXX lines", better add a rule like "avoid methods violating the SRP".

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
1

A method is too long that does more than it needs to do. Any blanket statement that "any method longer than XXX lines/statements is too long" is bogus, shows only that the author of the statement is a mindless acedemic with no experience writing actually productive software.

It's all nice and dandy to claim that "anything that can be must be removed into another method" but if that makes the code harder to follow it's not the thing to do (and it might introduce performance problems which is even worse). Again, the theoretician can make such statements, the professional has to work with reality and reality may well clash with book theory.

I've worked on fixing systems that were created according to "standards" set by such theoreticians. Systems that on paper were marvels of beauty and design, but in practice were next to impossible to maintain and failed to perform (mostly because they were way too slow because of an overly high load due to excessive method calls and object creations).
Worst case of that was a system that took 72 hours to generate 500 form letters. When bypassing the mess of the system framework and linking the letter generator directly to the database, that went down to about 20 seconds (not counting the time needed for the physical printing which was another few minutes).
That's the price you pay if you let theoretical correctness take front seat over pragmatic and practical design.

jwenting
  • 9,783
  • 3
  • 28
  • 45
  • I thing many people over engineer code using constructs they don't need and that make code unreadable. But usually I believe that methods of 5 / 10 lines with a speaking title make the code very readable. Make checks easier, obligates developer to specify the parameters and split the responsability. Also, the performance fall-down of 1000 more function calls is less than 1 second. It's not a problem. – M.F05051985 Apr 11 '14 at 09:54
  • 1
    @M.F05051985 it can be a problem. Worst case of it I've encountered is a loop doing an integer addition that was being iterated millions of times. There were dozens such loops in the application. Some overzealous fool had made a function (no kidding) int add(int a, int b){int c=a+b; return c;} and substituted calls to that for each addition in those loops. And no, you can't always cut something up in 5-10 line chunks and not lose readability. Far from it, quite often you lose readability. – jwenting Apr 11 '14 at 09:59
  • Obviously, your compiler is quite bad in inlining. In Java, I wouldn't hesitate to write such a function (it's free), if it made sense (which here obviously doesn't). The exact method line number don't matter, but I'd never commit or approve anything over 100 lines and even this only with simple code. This is a very generous limit and there's no reason not to split such monsters. I was unlucky enough to work with code having 1000+ line methods. ;) – maaartinus Jun 23 '17 at 01:00
  • @maaartinus sometimes you need longer methods, like when mapping from incoming data onto your internal database structures. It doesn't always make sense in the business logic to split that into multiple methods for a single record, making such a split "feel wrong" to anyone knowing the domain. – jwenting Jun 23 '17 at 05:51