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.

Does this answer your question? [How to tackle a 'branched' arrow head anti-pattern?](https://softwareengineering.stackexchange.com/questions/205803/how-to-tackle-a-branched-arrow-head-anti-pattern) — gnat, Feb 23 '21 at 20:12
@gnat: sorry, but I think that other question is a pointer in a completely wrong direction for this question, all suggested solutions are overcomplicated for this case, and none of them adresses how to avoid the duplication issue. — Doc Brown, Feb 23 '21 at 22:39

Doc Brown · Accepted Answer · 2021-02-24T06:01:55.440

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.

Wow, thank you very much for taking your time to write all this, it's super helpful. I will definitely try the func3 approach with my production code and post the results here. Additionally, many thanks for the b1 flag note and the code review link :) I appreciate it very much. — Codify, Feb 24 '21 at 01:50
I'm currently playing with your solution, and the blocker I faced is in the ' func3 { ... specificJobs(i1,item); ... } '- what if we need to pass a different set of args here? I was thinking about passing a dynamic object, but it becomes a bit more complex than needed. Thank you! — Codify, Feb 24 '21 at 18:45
@Codify: as I wrote in my answer, your example code is probably too alienated. I would recommend to extract a working part of your *real* program, check if you need to anonymize some names, and then ask about it on codereview.SE. — Doc Brown, Feb 24 '21 at 21:06

score 3 · Answer 2 · answered Feb 23 '21 at 22:59

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);

Thank you, John, it's an interesting approach and I'll definitely take a closer look at it. Much appreciated! — Codify, Feb 24 '21 at 02:33

score 0 · Answer 3 · answered Feb 23 '21 at 21:17

0

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();
    }

answered Feb 23 '21 at 21:17

FrustratedWithFormsDesigner

46,105
7
126
176

Thank you for your answer, it helps to avoid duplication for sure but doesn't solve the core issue here - other parts of the code are still duplicated. – Codify Feb 23 '21 at 21:32

score -1 · Answer 4 · answered Feb 23 '21 at 21:20

-1

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.

answered Feb 23 '21 at 21:20

gnasher729

42,090
4
59
119

Thank you for your answer, so we don't care if private methods have multiple responsibilities? It kinda solves my problem if that's the case. I can pass an enum and have a simple switch/case clause to account for multiple scenarios. – Codify Feb 23 '21 at 21:33
Ok, there will be people respecting the "single responsibility" principle, and others are totally obsessed with it. Your public functions have a single responsibility, that's good. Your private functions are not totally unimportant, but much less important because only you know. So weighing in that "don't repeat yourself" is also important, I'd value "DRY" higher in this case. Someone obsessed with SRP will contradict, but that won't give you better, more maintainable code. – gnasher729 Feb 23 '21 at 22:15
gnasher729 - thank you, it makes perfect sense to me, if it solves the problem and doesn't look ugly - why not :) – Codify Feb 24 '21 at 02:30

Refactoring two methods with similar for loops containing some unique logic

4 Answers4