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

closed as primarily opinion-based by Bart van Ingen Schenau, jwenting, gnat, Blrfl, user40980 Apr 11 '14 at 11:56

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 2
    Wrong. 1st step : Write some unit tests. – Euphoric Apr 11 '14 at 9: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? – Blrfl Apr 11 '14 at 11:38
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".

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.

  • 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 9: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 9: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 1: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 5:51

Not the answer you're looking for? Browse other questions tagged or ask your own question.