2

Is the following design flawed?

Basically I have a main class used to create an object of a type and then return it as the main interface of the type. The problem with this is that each of the classes that implement the interface all have different properties that need to be set. So I then have to cast them back to their main type to set them. See the below code:

public MainClass
{
    public static IInterface CreateInstance(InstanceType instanceType)
    {
        IInterface interface = null;
        switch (instanceType)
        {
            case InstanceType.ClassA:
            interface = new ClassA;
            break;

            case InstanceType.ClassB:
            interface = new ClassB;
            break;
        }

        return interface;
    }
}

public interface IInterface
{
    string Method1();
}

public class ClassA : IInterface
{
    public string Property1 { get; set; }

    public string Method1()
    {
        //...
    }
}

public class ClassB : IInterface
{
    public int Property2 { get; set; }

    public string Method1()
    {
        //...
    }
}

This is then used like:

IInterface interface = MainClass.CreateInstance(InstanceType.ClassA);
ClassA class = interface as ClassA;
class.Property1 = "";

interface.Method1();

Is there a better way to do this? (In reality there are more methods and properties than this)

  • 2
    Short answer, yes it's bad. CreateInstance should be setting those properties as part of the instance creation. See @kai's answer for a more detailed one. – David Arno Jun 8 '16 at 11:13
6

The act of creating an instance like this is called a factory pattern. It's very common and has several advantages in some cases.

However, the main point of having a factory give you references to an abstract type (interface) is that the client code doesn't need to, nor should know about the different subtypes. The client is decoupled from both object creation and the classes themselves. This is lost when you downcast to ClassA in client code.

You still may reap some benefits of decoupling client code from object construction, but if you absolutely HAVE to downcast, then just make several methods on the factory instead (CreateClassA() : ClassA, CreateClassB() : ClassB, etc).

Try to avoid this though. Factories shine when they allow you to decouple completely from an object graph and just code to some common interface. If your factory is just a really convoluted way to call the constructor, then you will have gained nothing and added a lot of complexity.

1

As an alternative to providing a value from the enum InstanceType as an argument to switch which type of object to construct, you could supply a generic type parameter and use that parameter as the return type. E.g.:

class MainClass
{
    public static T CreateInstance<T>()
        where T: IInterface
    {
        // ...
    }
 }

You can keep the switch statement in CreateInstance (actually replaced by a sequence of if-statements) or you can use a static field in a generic class as a kind of lookup table:

class BuilderClass<T>
    where T: IInterface
{
    static readonly Func<T> _construct;

    // Initialise `_construct` for each type you want to build from the static Constructor.
    static BuilderClass()
    {
        if (typeof(T) == typeof(ClassA))
        {
            BuilderClass<ClassA>._construct = () => new ClassA();
        }
        else if (typeof(T) == typeof(ClassB))
        {
            BuilderClass<ClassB>._construct = () => new ClassB();
        }
        else
        {
            BuilderClass<T>._construct = null;
        }
    }
}

CreateInstance can call BuilderClass<T>._construct():

public static T CreateInstance<T>()
    where T: IInterface
{
    System.Diagnostics.Debug.Assert(
        BuilderClass<T>._construct != null,
        $"Cannot not build objects of type {typeof(T).Name}");
    return BuilderClass<T>._construct();
}

But you should still think about kai's answer and whether you should be setting the type specific properties, after you have built the IInterface-objects.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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