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
9
@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”.
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.
4