C# Refactoring: How would you refactor 2 methods that do the same except that they use a different Interface

Question

Having 2 different interfaces is a must.

How would you refactor this?

Should I refactor this code at all?

private void CreateInstanceForProviderA()
{
    a = FactorySingleton.Instance.CreateInstanceA("A");

    if (a == null)
    {
        ShowProviderNotInstanciatedMessage();
        return;
    }

    a.Owner = Handle.ToInt32();
    lbl_Text.Text = a.Version();
}

private void CreateInstanceForProviderB()
{
    b = FactorySingleton.Instance.CreateInstanceB("B");

    if (b == null)
    {
        ShowProviderNotInstanciatedMessage();
        return;
    }

    b.Owner = Handle.ToInt32();
    lbl_Text.Text = b.Version();
}

If there would be a common interface, I could write:

private void CreateInstanceForProvider(string provider)
{

    p = FactorySingleton.Instance.CreateInstanceB(provider);
    // p is shared over the whole class
    if (p == null)
    {
        ShowProviderNotInstanciatedMessage();
        return;
    }

    var tmpProvider = p as ICommonProvider;

    tmpProvider .Owner = Handle.ToInt32();
    lbl_Text.Text = tmpProvider .Version();
}

Answer 1

I would leave it as it is, no refactoring necessary...
YET!
If/when ProviderC shows up, then I would refactor. 8 )

Answer 2

Perhaps have a third method that is private, and replace your code with calls to this third method, as so:

private void CreateInstanceForProviderA() { return DoSomething(); }

private void CreateInstanceForProviderB() { return DoSomething(); }

Answer 3

It all depends on those method calls (.CreateInstanceA and .CreateInstanceB).

If they're doing the same things in the same way, and the only difference is that string parameter, then yes: refactor the method to CreateInstanceForProvider(string providerCode) and let the user / calling code pass in the proper parameter.

If they do slightly different things, then you may still be able to refactor, and it becomes more of a headache. At that point, you have to determine whether the increased abstraction (and additional ease of adding new Providers) is worth refactoring and having to re-run (and possibly rewrite) whatever tests are necessary.

Answer 4

Well, the first thing to do is to yell at the author of FactorySingleton to fix has damn code so that ClassA and ClassB have a common interface for their common fields.

In the meantime, you pretty much stuck using reflection, which would be ugly and not worth it for just that little bit.

Answer 5

EDIT: This does no work because a and b are different types (not sure if that was before or after I gave my answer...)

I'm assuming a and b are fields, not properties.

Abstractly, put the common functionality in a single method called by both of the original methods:

private void CreateInstanceForProviderA()
{
    a = CreateInstanceForProvider("A");
}

private void CreateInstanceForProviderB()
{
    b = CreateInstanceForProvider("B");
}

private FactorySingleton CreateInstanceForProvider(string which) 
{
    var instance = FactorySingleton.Instance.CreateInstanceB(which);

    if (instance == null)
    {
        ShowProviderNotInstanciatedMessage();
        return;
    }

    instance.Owner = Handle.ToInt32();
    lbl_Text.Text = instance.Version(); 

    return instance;
}

Answer 6

I'd pass an enum in to specify the create instance code to call. After that create a wrapper class containing methods to get/set using reflection as suggested in other answers use reflection. Not sure it's worth it, as you code is probably harder to read than before.

    public enum Provider
    {
        A,
        B
    }

    private void CreateInstanceForProvider(Provider provider)
    {
        ProviderWrapper provider = null;

        switch (provider)
        {
            case Provider.A:
                provider = new ProviderWrapper(FactorySingleton.Instance.CreateInstanceA("A"));
                break;
            case Provider.B:
                provider = new ProviderWrapper(FactorySingleton.Instance.CreateInstanceB("B"));
                break;
        }

        if (provider == null)
        {
            ShowProviderNotInstanciatedMessage();
            return;
        }

        provider.SetOwner(Handle.ToInt32());
        lbl_Text.Text = provider.GetVersion();
    }

    public class ProviderWrapper
    {
        private readonly object _original;

        public ProviderWrapper(object original)
        {
            _original = original;
        }

        public void SetOwner(int value)
        {
            _original.GetType().GetProperty("Owner").SetValue(_original, value, null);
        }

        public string GetVersion()
        {
            return (String)_original.GetType().GetProperty("Owner").GetValue(_original, null);
        }
    }

Answer 7

Remove the duplicate code. In this case, you should be able to remove the middle message block that does the null check and message for a failed instantiation.

private void CreateInstanceForProviderA()
{
    a = FactorySingleton.Instance.CreateInstanceA("A");

    if (IsNullObject(a))
    {
       return;
    }

    a.Owner = Handle.ToInt32();
    lbl_Text.Text = a.Version();
}

private void CreateInstanceForProviderB()
{
    b = FactorySingleton.Instance.CreateInstanceB("B");

    if (IsNullObject(b))
    {
       return;
    }

    b.Owner = Handle.ToInt32();
    lbl_Text.Text = b.Version();
}

private bool IsNullObject(object obj)
{
    if (obj == null)
    {
        ShowProviderNotInstanciatedMessage();
        return true;
    }
    return false;
}

If you do find a way to provide a common interface or shared virtual methods on these providers, we can refactor more aggressively.

Answer 8

What version of C# are you using?

In C# 4 (Visual Studio 2010) the new dynamic keyword could help share code in this case. I wouldn't use if it this is a performance-critical section of code though, but if this just runs a handful of times then go ahead.

Answer 9

InstanceA and InstanceB ought to implement a common interface.

public interface IA : ICommon {...}
public interface IB : ICommon {...}

public interface ICommon
{
    int Owner {get;}
    string Version();
}

This way, you still have two different interfaces, but the common aspects of those interfaces are defined in a way that you can do some of the same things with both of them.

Answer 10

public interface ICommon
{
    int Owner { get; }
    string Version();
}

public interface IA : ICommon
public interface IB : ICommon

private void CreateInstanceForProvider(ICommon c) 
{ 
    if (c == null) 
    { 
        ShowProviderNotInstanciatedMessage(); 
        return; 
    } 

    c.Owner = Handle.ToInt32(); 
    lbl_Text.Text = c.Version(); 
} 

Answer 11

It's always painful to fight the type system. Without using dynamic, here goes my attempt.

Given that you have these two distinct interfaces for a and b:

interface IA {
  int Owner { set; }
  string Version();
}
interface IB {
  int Owner { set; }
  string Version();
}

You can create a wrapper type like this:

class WrapperAB : IA, IB {
  IA a; IB b;
  public WrapperAB(object o) {
    if (o is IA) a = (IA)o;
    else if (o is IB) b = (IB)o;
    else throw new Exception();
  }
  public int Owner {
    set { 
      if (a != null) a.Owner = value; 
      else b.Owner = value; 
    }
  }
  public string Version() {
    if (a != null) return a.Version();
    else return b.Version();
  }
}

And change your methods to this:

private void CreateInstanceForProviderA() {
  CreateInstanceForProvider<IA>("A", FactorySingleton.Instance.CreateInstanceA, out a);
}

private void CreateInstanceForProviderB() {
  CreateInstanceForProvider<IB>("B", FactorySingleton.Instance.CreateInstanceB, out b);
}

private void CreateInstanceForProvider<TI>(string name, Func<string, TI> factory, out TI instance) {
  instance = factory(name);

  if (instance == null) {
    ShowProviderNotInstanciatedMessage();
    return;
  }

  var w = new WrapperAB(instance);
  w.Owner = Handle.ToInt32();
  lbl_Text.Text = w.Version();
}