Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have 2 methods that do almost the same things except a few lines of code. The sample is like below.

public void Method1()
{
    DoX();

    if (value)
    {
        var input = GetA();
        DoA1();
        DoA2(input);
    }

    DoY();
}

public void Method2()
{
    DoX();

    if (value)
    {
        var input = GetB();
        DoB(input);
    }

    DoY();
}

I have two questions for this code.
1. Is it a good idea to remove code duplication is this case?
2. How to remove the duplication without make it harder to read the code

share|improve this question
2  
ermmm, Perhaps you could post the actual code and see if someone could offer comment. Or perhaps this is better suited for SE?? –  dreza Nov 9 '12 at 4:21
    
I was suggested to ask this question on codereview.se (this site.) I cannot post the actual code here due to a policy. :( –  Anonymous Nov 9 '12 at 7:15
    
No problem. I thought SE might have been more appropiate, but I always get confused myself half the time. Hopefully you get enough answers to be helpful enough for you –  dreza Nov 9 '12 at 7:52
1  
I try to apply the [Rule of 3] (en.wikipedia.org/wiki/Rule_of_three_%28computer_programming%29) when coding. It's okay to copy once, if you copy twice it is time to refactor. –  Gene S Nov 9 '12 at 14:12

5 Answers 5

Based on your code as that is what we are reviewing a couple of suggestions.

  1. Pass in a delegate that handles the changeable part
  2. Use Template method pattern to implement the changing parts
  3. Use composition and DI to inject the different elements

Example: Using delegates

private void DoMethod12(Action doMyAction)
{
    DoX();
    doMyAction();
    DoY();
}

public void Method1()
{
    DoMethod12(_ =>
    {
        var input = GetA();
        DoA1();
        DoA2(input);
    });
}

public void Method2()
{
    DoMethod12(_ =>
    {
       var input = GetB();
       DoB(input);
    });
}

Example 2: Template pattern

public abstract class MyDo
{
    protected abstract DoMyAction1();
    protected abstract DoMyAction2();

    public void Method1()
    {
        DoMethod12(doMyAction);
    }

    public void Method2()
    {
        DoMethod12(doMyAction2);
    }

    private void DoMethod12(Action doMyAction)
    {
        DoX();
        doMyAction();
        DoY();
    }
    private void DoX()
    {
    }
    private void DoY()
    {
    }
}

public class MyDoer : MyDo
{
    protected override DoMyAction1()
    {
           var input = GetA();
           DoA1();
           DoA2(input);
    }

    protected override DoMyAction2()
    {
       var input = GetB();
           DoB(input);
    }
}

Example 3: DI and composition

public interface IMyDoer
{
    void MyMethod1();
    void MyMethod2();
}

public class MyDoer
{
   private MyDoer _doer;

   public MyDoer(IMyDoer doer)
   {
      _doer = doer;
   }

    public void Method1()
    {
        DoMethod12(_doer.MyMethod1);
    }

    public void Method2()
    {
        DoMethod12(_doer.MyMethod2);
    }

    private void DoMethod12(Action doMyAction)
    {
        DoX();
        doMyAction();
        DoY();
    }

    private void DoX()
    {
    }
    private void DoY()
    {
    }
}

These written in notepad so no guareentee on exact syntax although hopefully they can give some ideas.

share|improve this answer
    
I really like that answer. It provides three different interesting ways to solve it using different design patterns. –  user19024 Nov 9 '12 at 13:51

One thing you could do is to create a common function that contains the DoX(), if(value), and DoY() logic, and takes a delegate as a parameter. That way you have the common logic encapsulated in one place but can still specify the piece specific to each method. For example:

    //making value a property so it's defined somewhere :)
    public bool Value { get; set; }

    //defining all of the methods called above
    public string GetA() { return ""; }
    public string GetB() { return ""; }
    public void DoA1() { }
    public void DoA2(string input) { }
    public void DoB(string input) { }
    public void DoX() { }
    public void DoY() { }

    //this delegate will hold the part of the original Method1/Method2 that is inside the if statement
    public delegate void DoDelegate();

    //this is the part inside the if statement of the original Method1
    public void Method1Specific() {
        var input = GetA();
        DoA1();
        DoA2(input);
    }

    //this is the part inside the if statement of the original Method2
    public void Method2Specific() {
        var input = GetB();
        DoB(input);
    }

    //this is the common part of both methods
    //it takes a parameter indicating what to run for the specific part inside the if statement
    public void Method(DoDelegate Do) {
        DoX();

        if (Value) {
            Do();
        }

        DoY();
    }

    //this would replace the existing Method1
    public void Method1() {
        Value = true;
        Method(Method1Specific);
    }

    //this would replace the existing Method2
    public void Method2() {
        Value = false;
        Method(Method2Specific);
    }

I commented this code to show what is actually happening here.

In this code, you will still call Method1 and Method2, just like before, but each of those methods just calls the Method method passing in the method appropriate for each. To extend this for more methods, just create (for example) a Method3Specific method that would do the logic specific to the new method, and then create a Method3 that passes in Method3Specific as the delegate. As you can see, once this structure is set up, it is easy to extend it to any number of methods.

share|improve this answer

Some things that I noticed that weren't talked about in previous answers that make me lean to delegates and/ or funcs:

  1. You're always calling a Get*N* before calling your Doers. Why not delegate that as well?
  2. The example has DoA1 with no parameters and DoA2 with parameters. If you can reconcile their signatures, you have a strong case for using delegates for all of your doers.

These ideas are similar to @dreza's first example, but the execution is different. Here's a small program for you to examine:

internal class Program
{
    private delegate void Doer(string input); 
    private static void Main()
    {
        MethodRigger();
    }

    private static void MethodRigger()
    {
        Doer doers = DoA1 + DoA2;
        Func<string> getInput = GetA;
        bool value = true ;  
        if (!value)
        {
            getInput = GetB;
            doers = DoB;
        }
        work(getInput, doers); 
    }

    private static void work(Func<string> getInput, Doer doers)
    {
        DoX();
        var input = getInput();
        doers(input);
        DoY();
    }

    public static void DoX(){}
    public static void DoY(){}

    public static string GetA(){ return string.Empty;}
    public static string GetB() {return string.Empty;}

    private static Doer DoA1 = (input) => { };
    private static Doer DoA2 = (input) => { };
    private static Doer DoB = (input) => { };
}
share|improve this answer

Yes your code needs few re-factoring.Always remember encapsulate logic in one place if it varies.

try this way and include a new param bool and when you need to include it pass true else false

public void Method1(bool IncludeDoA1)
    {
        DoX();

        if (value)
        {
            var input = GetA();
            if (IncludeDoA1) DoA1();
            DoA2(input);
        }

        DoY();
    }
share|improve this answer
    
What if I have 5 duplicate methods? –  Anonymous Nov 9 '12 at 5:32
2  
oohhh, don't know if I'm a fan of the boolean flag. Check this out for more info of discussion around it martinfowler.com/bliki/FlagArgument.html –  dreza Nov 9 '12 at 6:15
    
@Anonymous do you have 5 duplicate methods? –  dreza Nov 9 '12 at 6:16
    
@Anonymous why you want to write the 5 duplicate method , take my advise write few unit test cases for you code first and try to refactor it –  paritosh Nov 9 '12 at 6:22
    
@dreza There are 2 methods in the real code. I threw my comment because I don't think passing boolean flag is a good idea. Thanks for theh link to Fowler's website! –  Anonymous Nov 9 '12 at 6:42

For this specific code example, I see that there is some open-close pattern in the functions. If this is more relevant to your actual problem and is prominent in code then maybe you could look into AOP framework to help you define pointcut.

You could also try Func<>, Action<>, delegate to be passed as parameters and define the recipe using some dictionary, like a state machine if that is the case in the example.

On the other hand if your methods differ in the way that they call functions at different locations then I would suggest to keep them separate and not try to make one function with if's and else's that will not help readability of the code.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.