Use local or nested function for readability?

Question

I found in the catalog of Refactoring by Martin Fowler, with Kent Beck book that they mention Extract Function refactoring.

1. It is a good practice to wrap your related code into local functions to make easy the readability and maintenance of code?
   I always thought that local functions were created to avoid writing multiple times the same code inside some particular function that has to repeat some lines. One example can be for example modifying a string value like in the following example.

private void CopyOneFileToOtherInSomeDirectory(string path, string filename, string outputFilename)
{
    // Reading document
    var reader = File.OpenText($"{AppendPathSeparator(path)}{filename}");
    var text = reader.ReadToEnd();

    // Writing
    using (FileStream fs = File.Create($"{AppendPathSeparator(path)}{outputFilename}"))
    {
        byte[] textBytes = new UTF8Encoding(true).GetBytes(text);
        fs.Write(textBytes, 0, textBytes.Length);
    }

    string AppendPathSeparator(string filepath)
    {
        return filepath.EndsWith(@"\") ? filepath : filepath + @"\";
    }
}

but in the Extract Function refactoring extracting to local function can be applied to refactor the last example to something like this.

private void CopyOneFileToOtherInSomeDirectory(string path, string filename, string outputFilename)
{
    string Read()
    {
        var reader = File.OpenText($"{AppendPathSeparator(path)}{filename}");
        return reader.ReadToEnd();
    }
    void Write(string text)
    {
        using (FileStream fs = File.Create($"{AppendPathSeparator(path)}{outputFilename}"))
        {
            byte[] textBytes = new UTF8Encoding(true).GetBytes(text);
            fs.Write(textBytes, 0, textBytes.Length);
        }
    }
    string AppendPathSeparator(string filepath)
    {
        return filepath.EndsWith(@"\") ? filepath : filepath + @"\";
    }

    var readedText = Read();
    Write(readedText);
}

It looks bigger but with one of these awesome IDE like the IntelliJ ones you can collapse the inner methods and get something really clear to follow.

private void CopyOneFileToOtherInSomeDirectory(string path, string filename, string outputFilename)
{
    string Read(){...}
    void Write(string text){...}
    string AppendPathSeparator(string filepath){...}

    var readedText = Read();
    Write(readedText);
}

What do you think? is it a good practice to chop the functions with multiple steps to inner functions?
2. Can this have some impact in performance terms?

Answer 1

I always thought that local functions were created to avoid writing multiple times the same code

No. That's a misconception, outdated since the 1970s, and I'm happy that we have since learned a few things.

Refactoring code chunks into functions is an example of following the guideline

We introduce classes/methods/functions etc. to create the language we want to be using to express our solution.

So, this refactoring certainly is the right thing to do, if the language thus introduced better expresses the solution steps.

But, is the following

private void CopyOneFileToOtherInSomeDirectory(string path, string filename, string outputFilename)
{
    [...]
    var readedText = Read();
    Write(readedText);
}

really the way you want to express the steps of copying a file?

E.g., is Read() a good abstraction vocabulary for what it does? I don't think so. It lacks clarity, as you don't see where it reads from (filename in path), a very important information. So, to understand what it does, you still have to look into the function, and that means it's less readable (IMHO) than the original non-refactored version.

Is AppendPathSeparator(path) part of a good language for expressing the file-copy task? It's a helper dealing with one aspect of path syntax, necessary only in the context of combining a directory name and a file name. What you really want to do there is this combining of directory and file, and the aspect that you might or might not need an additional slash should be abstracted deep into some lower level of language.

If I were to create the language for the file copying function, I'd prefer something like:

private void CopyOneFileToOtherInSomeDirectory(string path, string filename, string outputFilename)
{
    var readedText = Read(Combine(path, filename));
    Write(readedText, Combine(path, outputFilename));
    [...]
}

Some general ideas:

• Language should be clear: when reading it, you should get a good idea of what happens there. That's what I missed in the Read() and Write(text) words.
• Language should use a consistent abstraction layer. If you create abstract vocabulary for some aspects of pathname syntax, you should cover all aspects.
• Language should be free of redundancy. Having multiple synonyms that express more or less the same concept is nice in literature, but not in software.
• Language should be unambiguous: a word like Read() should mean the same thing wherever you encounter it.

When following these guidelines, the ExtractFunction refactoring greatly improves code quality.

Regarding performance: you can safely ignore the tiny performance impact of this refactoring until you really run into a performance problem.

Optimize for performance only if it is necessary. That can't be over-emphasized.

And all real-world performance problems I ever experienced came from other sources, never ever was it an additional level of function calls.

Answer 2

is it a good practice to chop the functions with multiple steps to inner functions?

Yes.

Can this have some impact in performance terms?

Yes. But no one cares.

The biggest problem here is that the function definitions come before their use. Which means your eyes have to jump around to read it. Fix that and you’ll have much more readable code. But even if you can’t follow the step down rule, decomposed still reads better than procedural so long as you get one extremely important thing right.

The names.

Names will make or break this plan. Use good names and it’s easy to follow. Don’t and I’d rather be back in procedural hell.

Answer 3

If you have 1000 lines of code in one function it’s most likely better to split it up into 2 functions of slightly over 500 lines and a small function calling them. Same if you have a 100 line function. At 30 lines it gets dubious. For five lines, the mental overhead of having to read two functions plus the calls costs more than you save by having smaller functions. Your mileage may vary.

Answer 4

Usually and obviously, the goal of extracting a bunch of code into a function is to reuse the code, (by calling multiple times, rather than copy&paste).

Extracting function also improves the readability, but only when doing right.

A good method is a method that when you look at its signature, you already know all the info you need to know about it, by a glance:

• the function name shall express the purpose of this method;
• the parameters shall indicate ALL the inputs it needs;
• the return value, (plus optional ref, out parameters), indicates ALL the outputs.

It well encapsulates the complexity inside, reduce your brain burden when reading the code.

The bad example is local methods. You only know the purpose from the method name, but you don't know its inputs/outputs. Because it may capture variables in the outer method, you don't know which variables it needs, which variables it changes, unless you read through the whole function.

A good practice is to put all the variables needed in the parameter list. But, when you reading the code, you'd better keep in mind that it's not guaranteed no variable be captured inside a local method, especially when the local method isn't just few lines long, and written by others.