C# Assign a delegate based on a type - refactor help?

Question

Hi All, I had a quick question. Something seems really wrong with this code. I want to take advantage of generics and delegates and quite possibly generic delegates if applicable. I am working with some code generated api's and the objects generated are very similiar. I see that they all implement an interface so I was to try to create one class with a few methods to handle the different scenarios. Here is some sample code. It just feels wrong on so many levels. Please tell me how I can make this code better. A little refactoring advice if you will. And by all means rip it to shreds. I want to code better and learn to do things correctly.

Thanks, ~ck in San Diego

private delegate IsomeEntity DisplayDelegate(IsomeEntity display);

public IsomeEntity Display<T>()
{
    DisplayDelegate _del = null;
    IsomeEntity display = factory.CreateObject(typeof(T).Name);

    if (display.GetType() == typeof(ADisplayEntity))
        _del = ADisplayEntity;

    if (display.GetType() == typeof(BDisplayEntity))
        _del = BDisplayEntity;

    if (display.GetType() == typeof(CDisplayEntity))
        _del = CDisplayEntity;


    return _del(display);
}

public ADisplayEntity ADisplayEntity(IsomeEntity display)
{
    ADisplayEntity ade = display as ADisplayEntity;

    try
    {
        ADisplay o = new ADisplay();
        ADisplayEntity response = o.ADisplay(ade);
        return response;
    }
    catch (Exception ex)
    {
        Exception newEx;
        if (someExceptionHandler.HandleException(ex, this, out newEx))
            throw newEx;
    }

    return null;
}

public BDisplayEntity BDisplayEntity(IsomeEntity display)
{
    BDisplayEntity dde = display as BDisplayEntity;

    try
    {
        BDisplay o = new BDisplay();
        BDisplayEntity response = o.BDisplay(bde);
        return response;
    }
    catch (Exception ex)
    {
        Exception newEx;
        if (someExceptionHandler.HandleException(ex, this, out newEx))
            throw newEx;
    }

    return null;
}

Answer 1

You can pass delegates and lambda functions as the generic values, so something like this:

public ISomeEntity Display<T>( Func<T, ISomeEntity> conversion )
{
    IsomeEntity display = factory.CreateObject(typeof(T).Name);
    return conversion(display);
}

Calling this would then be:

var myNewEntity = Display( 
    x => ADisplay.GetADisplay(x as ADisplayEntity) );

And so on.

I'm not entirely sure what you're trying to do - so my code probably isn't quite right, but it should give you and idea of how you can pass lambdas around.

You can even store them in dictionaries and look them up.

Answer 2

Ok, if I follow you correctly, ADisplayEntity and BDisplayEntity are both generated classes that look pretty much the same, right? And you're trying to create some sort of generic delegate factory method (the one you called Display<T>) that allows the caller to specify the type of display entity they want, cast as a single interface you've created that the entities implement, yes? I take it that the ADisplay and BDisplay classes do not implement a common Display() method, which is why you have the calls to ADisplay and BDisplay. Hmm... those are the same name as the class. Typo? What you have listed won't even compile.

I think a better example is needed. I realize you're trying to sanitize your code, but perhaps some real code with name changes might be better. I'd like to see what ADisplay, BDisplay, ADisplayEntity and BDisplayEntity really are.

That said, if the various A/BDisplay and A/BDisplayEntity classes are really so different that you cannot consolidate those delegate methods into a single method, then you're pretty much doing the only thing you can do to achieve your original goal.

Perhaps with more info I can provide a better answer, but I don't see much to refactor here. Aside from the names of your methods being the same names as the classes they instantiate and your calls to a method that's the same name as the class.

Answer 3

why not something like simple? why do you need delegates..etc

 public static ISomeEntity DisplayEntity(ISomeEntity display)
{

         ISomeEntity result;
           if (entity is ADisplayEntity)
            {
                ADisplay disp = new ADisplay();
                result = disp.ADisplayFunc();
            }
          if(entity is BDisplayEntity)
           {
                BDisplay disp = new BDisplay();
                result = disp.BDisplayFunc();
           }

    return result;
}

Of course it would make more sense if you could have your ADisplay and BDisplay also follow an interface, like IDisplay then you just need to return IDisplay.Display(ISomeEntity)

To follow on that you could wrap your Displays like this..

public interface IDisplay
{
    public ISomeEntity Display(ISomeEntity entity);
}

public class AWrappedDisplay: IDisplay
{
    public ISomeEntity Display(ISomeEntity entity)
    {
        ADisplay disp = new ADisplay();
        return disp.ADisplayFunc(entity);
    }

}

public class BWrappedDisplay : IDisplay
{
    public ISomeEntity Display(ISomeEntity entity)
    {
        BDisplay disp = new BDisplay();
        return disp.BDisplayFunc(entity);
    }

}

public static IDisplay Factory(Type t)
        {
           IDisplay disp = null;
           if (t == typeof(ADisplayEntity))
               disp = new AWrappedDisplay();

           if (t == typeof(BDisplayEntity))
               disp = new BWrappedDisplay();

            return disp;
        }

and then you can call your DisplayEntity like this

  public static ISomeEntity DisplayEntity(ISomeEntity display)
    {
        IDisplay disp = Factory(display.GetType());
        ISomeEntity newDisplayEntity = disp.Display(display);

        return newDisplayEntity;
    }

Answer 4

If you wanted to generalize those methods you could do something like this:

private delegate IsomeEntity DisplayDelegate<T>(IsomeEntity display);

    public IsomeEntity DisplayMethod<T>() where T : IsomeEntity
    {
        DisplayDelegate<T> _del = new DisplayDelegate<T>(DoDisplay<T>);
        IsomeEntity entity = factory.CreateObject(typeof(T).Name);

        return _del(entity);
    }   

    public IsomeEntity DoDisplay<T>(IsomeEntity entity)
    {
        try
        {
            Display<T> o = new Display<T>();
            Entity<T> response = o.Display(entity);
            return response;
        }
        catch (Exception ex)
        {
            if (someExceptionHandler.HandleException(ex, this, out newEx))
                throw newEx;
        }
    }

In this situation, there is really no need for the delegate, and you could call the DoDisplay directly.

    public IsomeEntity DisplayMethod<T>() where T : IsomeEntity
    {            
        IsomeEntity entity = factory.CreateObject(typeof(T).Name);

        return DoDisplay<T>(entity);
    }   

    public IsomeEntity DoDisplay<T>(IsomeEntity entity)
    {
        try
        {
            Display<T> o = new Display<T>();
            Entity<T> response = o.Display(entity);
            return response;
        }
        catch (Exception ex)
        {
            if (someExceptionHandler.HandleException(ex, this, out newEx))
                throw newEx;
        }
    }

Answer 5

I'm going to assume since you mentioned that stuff was autogenerated that modifying ISomeEntity is out the window (otherwise I'd suggest adding a Display() method to ISomeEntity and then just calling it directly on the entity through the interface. Each entity would implement their own version of Display()).

So, if I understand what your code is trying to do correctly (it's not really clear), I'd suggest creating an IDisplay interface, having ADisplay and BDisplay inherit from it. This interface would have a method Display() which takes ISomeEntity and returns ISomeEntity. However, if ADisplay.Display(ISomeEntity) receives a BDisplayEntity you would throw an exception.

Then, I'd create an IDictionary which you'd store as a field in the class that has that main Display method (I'm going to call it Displayer). This dictionary would store what IDisplay to use for each type (ie typeof(ADisplayEntity) -> new ADisplay()).

You could then add your main Display method to Displayer, but now make it return a generic T, since T is the type you are creating and returning. This method looks up what IDisplay it needs and uses it on the factory created ISomeEntity and the result of which is returned.

The use of the Dictionary means that you don't get a crappy batch of if statements and that you can easily add more IDisplays by just adding to the dictionary.

Here's my code, which compiles in VS2008.

public interface ISomeEntity
{

}

public class EntityFactory
{
    public ISomeEntity CreateObject(string name)
    {
        //Do factory stuff here
        return null;
    }
}

public class ADisplayEntity : ISomeEntity
{
}



public class BDisplayEntity : ISomeEntity
{
}

public interface IDisplay
{
    ISomeEntity Display(ISomeEntity entity);
}

public class ADisplay : IDisplay
{
    public ISomeEntity Display(ISomeEntity entity)
    {
        ADisplayEntity aEntity = entity as ADisplayEntity;
        if (aEntity == null)
            throw new ArgumentException("Wrong type");

        //Do whatever happens when you convert parameter entity into a
        //"response" ADisplayEntity. I'm just returning a new 
        //ADisplayEntity to make it compile for me
        return new ADisplayEntity();
    }
}

public class BDisplay : IDisplay
{
    public ISomeEntity Display(ISomeEntity entity)
    {
        BDisplayEntity bEntity = entity as BDisplayEntity;
        if (bEntity == null)
            throw new ArgumentException("Wrong type");

        //Do whatever happens when you convert parameter entity into a
        //"response" BDisplayEntity. I'm just returning a new 
        //BDisplayEntity to make it compile for me
        return new BDisplayEntity();
    }
}



public class Displayer
{
    private IDictionary<Type, IDisplay> displayers;
    private EntityFactory factory;


    public Displayer()
    {
        factory = new EntityFactory();
        displayers = new Dictionary<Type, IDisplay>
                        {
                            { typeof(ADisplayEntity), new ADisplay() },
                            { typeof(BDisplayEntity), new BDisplay() }
                        };
    }


    public T Display<T>() where T : class, ISomeEntity
    {
        T entity = factory.CreateObject((typeof(T).Name)) as T; //Type-safe because of the factory
        IDisplay displayer = displayers[typeof(T)];
        return displayer.Display(entity) as T; //Typesafe thanks to each IDisplay returning the correct type
    }
}