Refactoring inner loops with lots of dependencies between levels

Question

I am having the following preudo-code

using (some web service/disposable object)
{
    list1 = service.get1();

    list2 = service.get2();

    for (item2 in list2)
    {
        list3 = service.get3(depending on item2);

        for (item3 in list3)
        {
            list4 = service.get4(depending on item3 and list1);

            for (item4 in list4)
            {
                ...
            }
        }
    }
}

where the entire code has, let's say 500 lines with lots of logic inside for statements. The problem is refactoring this into a readable and maintainable code and as a best practice for similar situations. Here are the possible solutions I found so far.

1: Split into methods Considering we extract each for into its own method, to improve readability, we end up with method2, method3 and method4. Each method has its own parameters and dependencies, which is good, except for method4. Method4 depends on list1 which means that list1 must be passed to methods 2 and 3 as well. From my opinion, this becomes unreadable. Any developer looking at method2 will realize there is no point for list1 inside it, so he has to look down the chain until method4 to actually realize the dependency -> inefficient. Then, what happens if list4 or item4 changes and no longer needs a dependency on list1 ? I have to remove list1 parameter for method4 (which should be done, naturally) but also for methods 2 and 3 (out of the scope of change) -> again inefficient. Another side effect is that, in case on lots of dependencies and multiple levels, the number of parameters passed down will quickly increase. Think what happens if list4 also depends on list11, list12 and list13, all created at the level of list1.

2: Keep a long single method The advantage is that every list and item can access every parent list and item, which makes the further changes a one-liner. If list4 no longer depends on list1, just remove/replace the code, without changing anything else. Obviously, the problem is that the method is couple hundred lines, which we all know it's not good.

3. Best of both worlds ? The idea is to split into methods only the inner logical part of each for. This way the main method will decrease and we gain readability and possibly maintainability. Leaving the for in the main method, we are still able to access each dependency as a one-liner. We end up with something like this:

for (item2 in list2)
{
    compute();

    list3 = service.get3(depending on item2);

    for (item3 in list3)
    {
        compute(item2, eventually item1)

        list4 = service.get4(depending on item3 and list1);

        for (item4 in list4)
        {
            ...
        }
    }
}

But there is another problem. How do you name those compute methods in order to be readable ? Let's say that I have a local variable instanceA or type ClassA which I populate from item2. Class A contains a property named LastItem4 which needs to keep, let's say, last item4 in order of creation date or whatever. If I use compute to create the instanceA, then this compute is only partial. Because LastItem4 property needs to be populated on item4 level, in a different compute method. So how to I call these 2 compute methods to suggest what I am actually doing ? Tough answer, in my opinion.

4. #region Leave a single long method but use regions. This is a feature strictly used in some programming languages and seems to me like a patch, not as a best practice, but maybe it's just me.

I would like to know how would you do this refactoring ? Be aware that it may be even more complex than that. The fact that is a service is a bit misleading, the idea is that i don't really like using disposable objects as method parameters, because you don't know the intent without reading the actual method code. But I was expecting this in comments since others may feel otherwise.

For the sake of example, let's assume that the service is a 3rd party one, without the possibility to change the way it works.

Answer 1

It seems you're looking for a general answer rather than a specific answer. Key to general answer is Separating the responsibilities and striving hard to achieve Single responsibility principle.

Let me start with the point that your method does toooooooo many things. This much stuff ideally a class also shouldn't do. It should be done with the help of group of classes.

Robert C.Martin used to say "Classes tends to hide in methods", which is the case here. If your method is big you can refactor it to smaller methods but if a local variable is needed throughout the method, probably that method should go in its own class.

If you need some fields to be access together, they have to live together(I mean in a same type). So all your List1, List2, List3, List4 should live in a class, not as method parameters whatsoever.

Given that you can't modify the Service class (assuming it is the third party API). That's fine, you can always create your own class which wraps the aforementioned service.

So you'll essentially a class something like the following:

public class ServiceWrapper
{
    private IList<string> list1;
    private IList<string> list2;
    private IList<string> list3;
    private IList<string> list4;

    private readonly Service1 service;
    public ServiceWrapper(Service1 service)
    {
        this.service = service;
    }

    public SomeClass GetSomething()
    {
        list1 = service.Get1();
        list2 = service.Get2();

        foreach (var item2 in list2)
        {
            PopulateMore(item2);
        }

        return result;//Return some computed result
    }

    private void PopulateMore(string item2)
    {
        list3 = service.Get3(item2);

        foreach (var item3 in list3)
        {
            PopulateEvenMore(item3);
        }
    }

    private void PopulateEvenMore(string item3)
    {
        list4 = service.Get4(item3);

        foreach (var item4 in list4)
        {
            //And so on
        }
    }
}

Given your service looks something like this

public class Service1 : IDisposable
{
    public void Dispose()
    {
        throw new NotImplementedException();
    }

    public IList<string> Get1()
    {
        throw new NotImplementedException();
    }

    public IList<string> Get2()
    {
        throw new NotImplementedException();
    }

    public IList<string> Get3(string item2)
    {
        throw new NotImplementedException();
    }

    public IList<string> Get4(string item3)
    {
        throw new NotImplementedException();
    }
}

So your very long method becomes very small now.

public void YourVeryLongMethodBecomesVerySmall()
{
    using (Service1 service = new Service1())
    {
        SomeClass result = new ServiceWrapper(service).GetSomething();
    }
}

As you said in comments if you have some other responsibilities also inside those methods you need to separate those responsibilities and move it in its own class. For example if you want to parse something, that should go inside some Parser class and ServiceWrapper should use the parser to get the job done (ideally with some abstraction and loose coupling).

You should extract methods and classes as much as you can so that it can't make sense to extract anymore.