C# Refactoring – Long Method: Leave As Is vs Separate into Methods vs Local Functions

cfunctionsmethodsrefactoring

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?

Best Answer

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.

Related Topic