Refactoring two methods with similar for loops containing some unique logic

Question

Please help me with the following code refactoring, I spent so much time researching it and there's still no clear answer on how to approach it. Any help is greatly appreciated!

So, given 2 methods (C#):

private void func1()
{
    int i1 = 0;
    bool b1 = false

    foreach (Object item in List<Object>)
    {
        if (!b1)
        {
            if (i1 == 0)
                doCommonJob1();
            else
                b1 = true;
            
            doCommonJob2();
        }

        // --- LOGIC 1 (could be whatever here)
        if (i1 == 10)
        {
            doSpecificJob1()
        }
        // ---

        doCommonJob3();
        i1++;
    }
}

And

private void func2()
{
    int i1 = 0;
    bool b1 = false

    foreach (Object item in List<Object>)
    {
        if (!b1)
        {
            if (i1 == 0)
                doCommonJob1();
            else
                b1 = true;
            
            doCommonJob2();
        }

        // --- LOGIC 2 (could be whatever here)
        if (b1 == true)
        {
            doSpecificJob2()
            doSpecificJob3()
        }

        doSpecificJob4()
        // ---

        doCommonJob3();
        i1++;
    }
}

There's so much duplication between them with some specific-to-method logic in the middle of a foreach loop.

It obviously smells, but making one method out of them and adding a boolean to the method's params would go against the single responsibility principle.

Any ideas on how to reuse the common code?

Thank you in advance.

Answer 1

You forgot to mention the programming language, I assume this is C#.

First things first: make the logic of those two methods less convoluted - get rid of this boolean flag b1, that will increase readability by an order of magnitude, which will make further refactoring a lot easier.

For example, your func2 can be written equivalently as

    private void func2(List<Object> list)
    {
        int i1 = 0;

        foreach (Object item in list)
        {
            if (i1 == 0)
                doCommonJob1();
            if (i1 <= 1)
                doCommonJob2();

            // --- LOGIC 2
            if (i1 >= 1)
            {
                doSpecificJob2();
                doSpecificJob3();
            }

            doSpecificJob4();
            // ---

            doCommonJob3();
            i1++;
        }
    }

Now, technically, you can get rid of the duplicate code by creating a new func3 of the following form:

    private void func3(List<Object> list, Action<int, Object> specificJobs)
    {
        int i1 = 0;

        foreach (Object item in list)
        {
            if (i1 == 0)
                doCommonJob1();
            if(i1<=1)
                doCommonJob2();

            specificJobs(i1,item);

            doCommonJob3();
            i1++;
        }
    }

which is used like this to replace func2:

        func3(list, (i1,item) =>
        {
            if (i1 >= 1)
            {
                doSpecificJob2();
                doSpecificJob3();
            }

            doSpecificJob4();
        });

(as an exercise to the reader: use func3 to replace func1).

For more complex scenarios, with several "specialized" parts interwoven with common parts, you can also utilize the template method pattern, as shown in John Wu's answer

However, IMHO your example code does not look sensible enough to be like any "real" code. Hence I am actually not sure if my suggestion is really suitable for your context, since the code in your real code base will most probably look somewhat different from what you posted here. I can also not tell you how to name func3 in a meaningful way, or if this will lead to a meaningful abstraction in your system, since names like func1 and func2 are not very meaningful names either. From what you presented in this contrived example it is unclear whether the coupling of func1 and func2 to a third function func3 is really desirable, or if it makes more sense to keep func1 and func2 independent from each other.

Final recommendation: post code which is closer to your "real code" at https://codereview.stackexchange.com/ and ask there for feedback, their community is way more specialized on code reviews than ours.

Answer 2

This is the sort of thing virtual methods are for. They allow you to swap out functionality in a larger algorithm.

First, create a "processor" base class and define the non-common code as abstract methods.

abstract class BaseProcessor
{
    protected void DoCommonJob1()
    {
        //Do common stuff
    }
    protected void DoCommonJob2()
    {
        //Do common stuff
    }
    protected void DoCommonJob3()
    {
        //Do common stuff
    }

    protected abstract void DoSpecificJob1();
    protected abstract void DoSpecificJob2();


    public void Process(List<object> list)
    {
        int i1 = 0;
        bool b1 = false;

        foreach (Object item in list)
        {
            if (!b1)
            {
                if (i1 == 0)
                    DoCommonJob1();
                else
                    b1 = true;

                DoCommonJob2();
            }

            DoSpecificJob1();
            DoSpecificJob2();

            DoCommonJob3();
            i1++;
        }

    }
}

Then, override the abstract methods in derived classes to do specific things.

class Type1Processor : BaseProcessor
{
    protected override void DoSpecificJob1()
    {
        //Do specific stuff 
    }
    protected override void DoSpecificJob2()
    {
        //Do specific stuff
    }
}
class Type2Processor : BaseProcessor
{
    protected override void DoSpecificJob1()
    {
        //Do other, specific stuff
    }
    protected override void DoSpecificJob2()
    {
        //Do other, specific stuff
    }
}

Once that is complete, you'd replace your calls to your old func1 with this:

var processor = new Type1Processor();
processor.Process(list);

And you'd replace your calls to your old func2 with this:

var processor = new Type2Processor();
processor.Process(list);

Answer 3

You could start with the code that seems to be identical between these two:

    if (!b1)
    {
        doNotBStuff()
    }

    // somewhere else..

    function doNotBStuff()
    {
        if (i1 == 0)
            doCommonJob1();
        else
            b1 = true;
        
        doCommonJob2();
    }

Answer 4

Just create a private method with the extra parameter, and then two public methods calling it. You have two public methods with a single responsibility, nobody cares about your private method. And you don't repeat yourself.