Refactoring: highlight the method when it makes sense

Now it’s hard to remember the moment when I first realized that it’s a good idea to select functions from large pieces of useful code. Whether I got this knowledge from “Perfect Code” , or from “Clean Code” is hard to remember. In general, this is not particularly important. We all know that we need to distribute business logic across well-named functions. The longest feature I've ever seen in my life was a 5k line of video. I am personally acquainted with the “programmer” who wrote that code. I remember the first time I met this function. It is not difficult to predict that my first reaction was: “What the hell !!! Who produced this piece of shit ??? ”
Yes, imagine, this “programmer” is still hanging around here in the office, where I am currently working on current projects. I do not want to delve into this story, but I want to mention that that function with a length of 5k lines was the core of the program, approximately 150k lines in size. The development of the program eventually came to a standstill, because of that terrible function that had a very negative effect on the architecture of the application. In the end, it was decided to rewrite the application from scratch.

This story illustrates one extreme of the problem of the size of functions, which led to disastrous consequences. At the other extreme, turn off the brain and start allocating classes with single-line functions inside. I do not mean that such functions are bad, I say that we should not forget to use the power of your brain. First, you should consider the problem.
Before I continue to explore the problem more deeply, I would like to note that, generally speaking, some time ago there was a small battle between Uncle Bob and Kristin Gorman on this topic. Uncle Bob presented a technique that he called “Extract till you drop” , which briefly means - extract functions as long as there is something to extract. Christine Gorman found that this technique eliminates the use of the brain . In addition, there was a John Sonmez post about refactoring a single function from .NET BCL (although the original purpose of the article was to show that most of the comments are evil).

Let's look at an example of refactoring done by John. He took for example the following method:

internal static void SplitDirectoryFile( string path, out string directory, out string file) { directory = null; file = null; // assumes a validated full path if (path != null) { int length = path.Length; int rootLength = GetRootLength(path); // ignore a trailing slash if (length > rootLength && EndsInDirectorySeparator(path)) length--; // find the pivot index between end of string and root for (int pivot = length - 1; pivot >= rootLength; pivot--) { if (IsDirectorySeparator(path[pivot])) { directory = path.Substring(0, pivot); file = path.Substring(pivot + 1, length - pivot - 1); return; } } // no pivot, return just the trimmed directory directory = path.Substring(0, length); } return; } 

')
In order to make this code easier to read, John created a new class by putting the refactored source method in it. Here is what he did:

 public class DirectoryFileSplitter { private readonly string validatedFullPath; private int length; private int rootLength; private bool pivotFound; public string Directory { get; set; } public string File { get; set; } public DirectoryFileSplitter(string validatedFullPath) { this.validatedFullPath = validatedFullPath; length = validatedFullPath.Length; rootLength = GetRootLength(validatedFullPath); } public void Split() { if (validatedFullPath != null) { IgnoreTrailingSlash(); FindPivotIndexBetweenEndOfStringAndRoot(); if(!pivotFound) TrimDirectory(); } } private void TrimDirectory() { Directory = validatedFullPath.Substring(0, length); } private void FindPivotIndexBetweenEndOfStringAndRoot() { for (int pivot = length - 1; pivot >= rootLength; pivot--) { if (IsDirectorySeparator(validatedFullPath[pivot])) { Directory = validatedFullPath.Substring(0, pivot); File = validatedFullPath.Substring(pivot + 1, length - pivot - 1); pivotFound = true; } } } private void IgnoreTrailingSlash() { if (length > rootLength && EndsInDirectorySeparator(validatedFullPath)) length--; } } 

Wow, yes? It’s not so easy to decide whether refactoring really helped make the code more readable. The feeling that, in fact, it has become harder to read. Previously, there was a relatively small function with helpful comments, which has now been turned into a class with four functions inside without comments. I would not say that the new class is bad and the whole refactoring was a bad idea, and the programmer who did the refactoring should be executed. Not at all. I'm not so bloodthirsty. There are several differences between these two code examples. Consider these differences:

1. If you are trying to achieve a deep understanding of what the top-level function does, the function has become more difficult to read than it was originally, because now you need to skip all the functions and understand what is happening in each of them. On the contrary, the initial version can easily be quickly run through the eyes.
2. If you are trying to understand what the top-level function is doing conceptually, then the refactor version is easier to read, because we immediately see what the function is doing inside of the concept.
3. The third difference I see is the cost of support. As for our specific example, I would say that the cost of supporting the refactored version is higher than the original one (at least you need to refactor). In general, the answer to the question of which option is more expensive to support lies in the plane of requirements. These requirements dictate whether it is important in this situation to follow the SRP (the principle of sole responsibility) or not. If this function can be written once and forgotten until the end of time, then there is no reason to waste time refactoring it. On the contrary, if it is expected that the functionality will grow, then you have every reason to refactor the function into a separate class.

In addition, I want to touch upon a situation where you accidentally (or deliberately) stumble upon a similar function in the legacy system. Do you immediately rush out to extract a class with four functions inside? My advice - do not do this for no good reason, even if test coverage of your code base tends to be 100%. Why? Because there is no technical duty. I'm talking about a serious technical duty, which is the cause of suffering.

Thus, there is nothing wrong with the “extract till you drop” technique. You just have to keep some thoughts in my head, in my opinion.
Summing up, I want to say that you should never commit meaningless acts. You must first think, analyze, draw a conclusion, and only after that act.