tags:

views:

77

answers:

2

I have factory that looks something like the following snippet. Foo is a wrapper class for Bar and in most cases (but not all), there is a 1:1 mapping. As a rule, Bar cannot know anything about Foo, yet Foo takes an instance of Bar. Is there a better/cleaner approach to doing this?

public Foo Make( Bar obj )
{
    if( obj is Bar1 )
        return new Foo1( obj as Bar1 );
    if( obj is Bar2 )
        return new Foo2( obj as Bar2 );
    if( obj is Bar3 )
        return new Foo3( obj as Bar3 );
    if( obj is Bar4 )
        return new Foo3( obj as Bar4 ); // same wrapper as Bar3
    throw new ArgumentException();
}

At first glance, this question might look like a duplicate (maybe it is), but I haven't seen one exactly like it. Here is one that is close, but not quite:

http://stackoverflow.com/questions/242097/factory-based-on-typeof-or-is-a

+4  A: 

If these are reference types, then calling as after is is an unnecessary expense. The usual idiom is to cast with as and check for null.

Taking a step back from micro-optimization, it looks like you could use some of the techniques in the article you linked to. In specific, you could create a dictionary keyed on the type, with the value being a delegate that constructs an instance. The delegate would take a (child of) Bar and return a (child of) Foo. Ideally, each child of Foo would register itself to the dictionary, which could be static within Foo itself.

Here's some sample code:

// Foo creator delegate.
public delegate Foo CreateFoo(Bar bar);

// Lookup of creators, for each type of Bar.
public static Dictionary<Type, CreateFoo> Factory = new Dictionary<Type, CreateFoo>();

// Registration.
Factory.Add(typeof(Bar1), (b => new Foo1(b)));

// Factory method.
static Foo Create(Bar bar)
{
    CreateFoo cf;
    if (!Factory.TryGetValue(bar.GetType(), out cf))
        return null;

    return cf(bar);
}
Steven Sudit
Right. I am aware that "is" is just a wrapped up "as" but having a long list of: Bar1 bar1 = obj as Bar1; if (bar1 != null ) return new Foo1( bar1 );just seems immensely ugly to me. ;-)
Swim
I'll keep watching this, because I don't know a better way than what you're doing and it's how I would solve this problem...
drachenstern
Swim, if you don't need the flexibility, then having a simple, fast but ugly method isn't so terrible. For flexibility, consider the dictionary technique.
Steven Sudit
Dear downvoter (presumably Stefan): It's fine for you to downvote, but I'd love it if you explained why.
Steven Sudit
Uhm, I didn't vote anything here and I never downvote without a comment. It's probably the same who downvoted me?
Stefan Steinegger
Back to the topic: The delegate is nice, independent of the optimization. It allows any constructor signature. But you need to register all the types. This gives a high coupling of the factory to these types. So it depends on the situation where it is used, if the delegates are more appropriate as the fully generic (reflection with "automatic type registering") way. At the end, it is not much of a difference. It's just an implementation detail.
Stefan Steinegger
@Stefan: My apologies for jumping to conclusions.
Steven Sudit
@Stefan: What I like about the Activator approach is that it lends itself well to specifying the type as a configurable string. That's why I've used it for things like plug-ins and, more generally, inversion of control. In those cases, the late-bound object implemented an interface. In the OP, we have children of a base class, so they're already coupled pretty tightly. Calling a static method in the base to register themselves doesn't seem like a lot of additional coupling. (continued)
Steven Sudit
Having said that, it would certainly be possible to combine the approaches to have the best of both worlds. In this case, it could use a dictionary, but if no delegate is found, it could fall back to Activator. So, yes, in the end, it's just an implementation detail and not a huge one.
Steven Sudit
+1  A: 

I'm not sure what you actually want to achieve. I would probably try to make it more generic.

You could use attributes on Foo, which Bar it supports, then you create a list in a initializing phase. We are doing quite a lot of stuff like this, it make adding and connecting new classes very easy.

private Dictionary<Type, Type> fooOfBar = new Dictionary<Type, Type>();
public initialize()
{
  // you could scan all types in the assembly of a certain base class
  // (fooType) and read the attribute

  fooOfBar.Add(attribute.BarType, fooType);
}

public Foo Make( Bar obj )
{
  return (Foo)Activator.CreateInstance(fooOfBar(obj.GetType()), obj);
}
Stefan Steinegger
@Stefan: I suggested something similar, but without the reflection. The value of the dictionary could be a delegate instead of a raw type.
Steven Sudit
@Stefan: I've added a code sample to make that more clear. While I haven't benchmarked it, I strongly suspect that the delegate approach will be faster than `Activator.CreateInstance`.
Steven Sudit
I just tried this approach and I like it very much. It is very clean and easily (trivially) maintained. Peak performance for this particular factory is not a necessity nor a requirement, otherwise, I would probably go with the response from @Steven, which I initially did.
Swim
@Swim: You don't have to decide. If you use a delegate, you can either wire in the constructor explicitly or create it through Activator.
Steven Sudit