Long method refactoring: leaving as is vs separating into methods vs using local functions

Question

Suppose I have long method like this:

public void SomeLongMethod()
{
    // Some task #1
    ...

    // Some task #2
    ...
}

This method doesn't have any repetitive parts that should be moved to separate method or local function.

There are many people (including me) who think that long methods are code smells. Also I don't like idea of using #region(s) here and there is a very popular answer explaining why this is bad.

---

But if I separate this code into methods

public void SomeLongMethod()
{
    Task1();

    Task2();
}

private void Task1()
{
    // Some task #1
    ...
}

private void Task2()
{
    // Some task #1
    ...
}

I see the following issues:

• Polluting class definition scope with definitions that are used internally by a single method and which mean I should document somewhere that Task1 and Task2 are intended only for internals of SomeLongMethod (or every person who reads my code will have to infer this idea).

• Polluting IDE autocomplete (e.g. Intellisense) of methods that would be used only once inside the single SomeLongMethod method.

---

So if I separate this method code into local functions

public void SomeLongMethod()
{
    Task1();

    Task2();

    void Task1()
    {
        // Some task #1
        ...
    }

    void Task2()
    {
        // Some task #1
        ...
    }
}

then this doesn't have the drawbacks of separate methods but this doesn't look better (at least for me) than the original method.

---

Which version of SomeLongMethod is more maintainable and readable for you and why?

Answer 1

Self-contained tasks should be moved to self-contained methods. There is no drawback large enough to override this goal.

You say that they pollute the namespace of the class, but that's not the trade-off you have to be looking at when factoring your code. A future reader of the class (e.g. you) is a lot more likely to ask "What does this specific method do and how must I change it so that it does what the changed requirements say?" than "What is the purpose and sense of being of all the methods in the class?" Therefore, making each of the methods easier to understand (by making it have fewer elements) is a lot more important than making the entire class easier to understand (by making it have fewer methods).

Of course, if the tasks are not truly self-contained but require some state variables to move along, then a method object, helper object or a similar construct can be the right choice instead. And if the tasks are totally self-contained and not bound up with the application domain at all (e.g. just string formatting, then conceivably they should go into a whole different (utility) class. But that all doesn't change the fact that "keeping methods per class down" is at best a very subsidiary goal.

Answer 2

I don't like either approach. You didn't provide any broader context but most of the times it looks like this:

You have a class that does some job by combining the power of multiple services into a single result.

class SomeClass
{
    private readonly Service1 _service1;
    private readonly Service2 _service2;

    public void SomeLongMethod()
    {
        _service1.Task1();
        _service2.Task2();       
    }
}

Each of your services implements only a part of the logic. Something special, that only this servcice can do. If it has other private methods than only ones that support the main API so you can sort them easily and there shouldn't be too many of them anyway.

class Service1
{
    public void Task1()
    {
        // Some task #1
        ...
    }

    private void SomeHelperMethod() {}
}

class Service2
{
    void Task2()
    {
        // Some task #1
        ...
    }
}

The bottom line is: if you start creating regions in your code in order to gorup methods, it's about time to start thinking about encapsulating their logic with a new service.

Answer 3

The problem with creating functions within the method is it doesn't reduce the length of the method.

If you want the extra level of protection 'private to method' then you can create a new class

public class BigClass
{
    public void SomeLongMethod();

    private class SomeLongMethodRunner
    {
        private void Task1();
        private void Task2();
    }
}

But classes in classes are a big ugly :/

Answer 4

Minimizing the scope of language constructs such as variables and methods is good practice. E.g. avoid global variables.. It makes the code easier to understand and helps avoiding misuse because the construct can be accessed in fewer places.

This speaks in favor of using the local functions.

Answer 5

I'd agree that long methods are often a code smell, but I don't think that's the whole picture, rather it's why the method is long that indicates a problem. My general rule is if the code is doing multiple distinct tasks then each of those tasks should be its own method. For me it does matter whether those sections of code are repetitive or not; unless you are really absolutely sure that no one would want to individually call these separately in the future (and that's a very hard thing to be sure of!) put them in another method (and make it private - which should be a very strong indicator you are not expecting it to be used outside of the class).

I must confess I've not used local functions yet, so my understanding is far from complete here, but the link you provided suggests they would often be used in a similar way to a lambda expression (although that here are a number of use cases where they may be more appropriate than lambdas, or where you can not use a lambda see Local functions compared to lambda expressions for specifics). Here then, I think it could down to the granularity of the 'other' tasks; if they are fairly trivial then perhaps a local function would be OK? On the other hand I have seen examples of behemoth lambda expressions (probably where a developer has recently discovered LINQ) which have made the code far less understandable and maintainable than if it had been simply called from another method.

The Local Functions page states that:

'Local functions make the intent of your code clear. Anyone reading you code can see that the method is not callable except by the containing method. For team projects, they also make it impossible for another developer to mistakenly call the method directly from elsewhere in the class or stuct.'

I'm not sure I'm really with MS on this one as a reason for use. The scope of your method (along with its name and comments) should make it clear what your intent was at that time. With a Local Function I could see that, other devs may see it as more sacrosanct, but potentially to the point where if a change comes in the future which requires that functionality to be reused they write their own procedure and leave the Local Function in place; thereby creating a code duplication issue. The latter sentence from the above quote could be relevant if you do not trust team members to understand a private method before calling it, and so call it inappropriately (so your decision could be affected by that, although education would be a better option here).

In summary, in general, I'd favour separating out into methods but MS wrote Local Functions for a reason, so I certainly wouldn't say don't use them,