views:

233

answers:

3

Hi All,

I am trying to refactor a piece of code which seems easily refactorable but is proving difficult. There are two method which seem very similar and I feel should be refactored:-

public class MyClass
{
    private void AddBasicData(Receiver receiver)
    {
        var aHelper = new AHelper();
        var bHelper = new BHelper();
        var cHelper = new CHelper();
        receiver.ObjA = aHelper.GetBasic();
        receiver.ObjB = bHelper.GetBasic();
        receiver.ObjC = cHelper.GetBasic();
    }

    private void AddExistingData(Receiver receiver)
    {
        var aHelper = new AHelper();
        var bHelper = new BHelper();
        var cHelper = new CHelper();
        receiver.ObjA = aHelper.GetExisting();
        receiver.ObjB = bHelper.GetExisting();
        receiver.ObjC = cHelper.GetExisting();
    }
}

The reference code for this class is here...

public class AHelper : Helper<A> 
{
}

public class BHelper : Helper<B> 
{
}

public class CHelper : Helper<C> 
{
}

public class Helper<T> : IHelper<T> where T : IMyObj
{
    public T GetBasic()
    {
       ...
    }

    public T GetExisting()
    {
       ...
    } 
}

public interface IHelper<T>
{
    T GetBasic();
    T GetExisting();
}

public class A : IMyObj {}
public class B : IMyObj {}
public class C : IMyObj {}

public interface IMyObj {}

public class Receiver
{
    public A ObjA { get; set; }
    public B ObjB { get; set; }
    public C ObjC { get; set; }
}

My first attempt was to refactor like this...

public class MyClass
{
    private void AddBasicData(Receiver receiver)
    {
        Func<Helper<IMyObj>, IMyObj> func = x => x.GetBasic();
        AddData(receiver, func);
    }

    private void AddExistingData(Receiver receiver)
    {
        Func<Helper<IMyObj>, IMyObj> func = x => x.GetExisting();
        AddData(receiver, func);
    }

    private void AddData(Receiver receiver, Func<Helper<IMyObj>, IMyObj> func)
    {
        var aHelper = new AHelper();
        var bHelper = new BHelper();
        var cHelper = new CHelper();
        receiver.ObjA = func(aHelper);
        receiver.ObjB = func(bHelper);
        receiver.ObjC = func(cHelper);
    }
}

The problem with this is objects like new AHelper() is not assignable to Helper<IMyObj> :-(

Can anyone see how this could be nicely refactored?

Thanks in advance

Russell

A: 

You can use casting for solving assigning problem. If AHelper actually return A, I think this works

private void AddData(Receiver receiver, Func<Helper<IMyObj>, IMyObj> func)
    {
        var aHelper = new AHelper();
        var bHelper = new BHelper();
        var cHelper = new CHelper();
        receiver.ObjA = (A) func(aHelper);
        receiver.ObjB = (B) func(bHelper);
        receiver.ObjC = (C) func(cHelper);
    }

if you override methods, you can do casting, dont need to change definition of "Func, IMyObj>"

public class AHelper : Helper<A> 
{
  public override A GetBasic()
  {
    return new A();
  }

}
Sessiz Saat
func can't take aHelper because func takes Helper<IMyObj> while AHelper is Helper<A>. Assigning return value is no issue
Ray
I mean that If you override methods in AHelper and return A, it seems like IMyobj due to function definition, but actuaaly its type A. So, you can cast and assign it to ObjA property of receiver
Sessiz Saat
yes, of course return value IMyObj can be converted at runtime to A because A implements IMyObj - thats not an actual issue.Actual issue is convering Helper<IMyObj> to Helper<A> Helper<B> Helper<C> respectfuly. In order to do this language need to support so called generic contravariance. See here for explanation http://blogs.msdn.com/wriju/archive/2009/07/31/c-4-0-co-variance-and-contra-variance.aspx
Ray
sorry my misunderstanding
Sessiz Saat
+2  A: 

Try using a templated function. It should infer the type based on the type of parameter you pass, so you shouldn't need to explicitly specify the type in the AddData call.

public class MyClass
{

  private void AddData<T>(Receiver receiver, Func<Helper<T>, T> func)
  {
     var aHelper = new AHelper();
     var bHelper = new BHelper();
     var cHelper = new CHelper();
     receiver.ObjA = func(aHelper);
     receiver.ObjB = func(bHelper);
     receiver.ObjC = func(cHelper);
  }
}

Attempt #2: Tricky problem I think you need a more generic IHelper interface. Would something like this help?

public interface IHelper
{
   IMyObj GetBasic();
   IMyObj GetExisting();
}

public interface IHelper<T> : IHelper
{
   T GetBasic();
   T GetExisting();
}

You'll have to work out the name conflict between the derived interface and the base interface, but I'm not sure exactly how you'd want to do that, and I'm running out of time, so I'll leave that as it for the moment.

Attempt #3 (I'm determined to get this!): Would this be cheating?

  public enum RetrievalMethod
  {
     Basic,
     Existing
  }
  public class Helper<T> : IHelper<T> where T : IMyObj
  {

     public T Get(RetrievalMethod rm)
     {
        switch(rm)
        {
           case RetrievalMethod.Basic:
              return GetBasic();
           case RetrievalMethod.Existing:
              return GetExisting();
        }
     }
...
  }
...
     private void AddData(Receiver receiver, RetrievalMethod rm)
     {
        var aHelper = new AHelper();
        var bHelper = new BHelper();
        var cHelper = new CHelper();
        receiver.ObjA = aHelper.Get(rm);
        receiver.ObjB = bHelper.Get(rm);
        receiver.ObjC = cHelper.Get(rm);
     }
BlueMonkMN
Oh wait, this won't work because T would have to be A, B and C, right?
BlueMonkMN
Yep, unfortunately :-/
Russell Giddings
How about Attempt #2
BlueMonkMN
I think replacing all 'T's with 'IMyObj's is the only way to achieve the refactoring in this instance. This would mean losing some of the type implications though and in this instance I dont think the benefit is worth that drawback. Thanks for putting your time and thought in to this. :-)
Russell Giddings
How about attempt #3?
BlueMonkMN
Yes it would be cheating, but those two options are as close as we're gonna get with .net 3.5 I think so I'm marking this as the answer, if for nothing more than your perseverance! ;-)
Russell Giddings
Is there any reason you don't want to "cheat" this way? Because I have more ideas. I would explore attempt number 2 further because it you have both strongly (T) and weakly (IMyObj) typed interfaces, you should be able to switch between them as needed without changing everything to IMyObj. Or you could pass the name of the function you want to call and use reflection to get the method. Or maybe there's a way to pass the MethodInfo of the functions of the un-filled generic type as parameters, and fill in T dynamically with System.Type.MakeGenericType().
BlueMonkMN
My aim here is to make the code more understandable and maintainable to save time and reduce the chance bugs creeping in in the future. I dont think any type of refactoring that is currently possible in 3.5 would meet all these criteria. In this situation any further reduction in duplicity to prevent bugs adds complexity reducing maintainability.Gees, thats enough long words for one day! ;-)
Russell Giddings
A: 

What about this?

private static T GetData<T, THelper>(Func<THelper, T> func) 
    where THelper : Helper<T>, new() 
    where T : IMyObj
{ return func(new THelper()); }

private static T GetBasicData<T, THelper>() 
    where THelper : Helper<T>, new() 
    where T : IMyObj
{ return GetData(x => x.GetBasic()); }

private static T GetExistingData<T, THelper>() 
    where THelper : Helper<T>, new() 
    where T : IMyObj
{ return GetData(x => x.GetExisting()); }

private void AddBasicData(Receiver receiver)
{
    receiver.ObjA = GetBasicData<A, AHelper>();
    receiver.ObjB = GetBasicData<B, BHelper>();
    receiver.ObjC = GetBasicData<C, CHelper>();
}

private void AddExistingData(Receiver receiver)
{
    receiver.ObjA = GetExistingData<A, AHelper>();
    receiver.ObjB = GetExistingData<B, BHelper>();
    receiver.ObjC = GetExistingData<C, CHelper>();
}
Alexey Romanov