Correct implementation of factory

Question

I'm trying to implement the factory pattern the correct way but I'm not sure if this is correct. I have three model classes derived from a base class like so:

class BaseStyle
{
    public string Name
    {
        get;
        set;
    }
}

class PointStyle : BaseStyle
{
    public PointStyle(string name)
    {
         Name = name;
    }
}

class LineStyle : BaseStyle
{
    public LineStyle(string name)
    {
         Name = name;
    }
}

class PolyStyle : BaseStyle
{
    public PolyStyle(string name)
    {
         Name = name;
    }
}

I then have a class called StyleFactory. This class takes a string to determine which type of Style to create and returns that Style.

public class StyleFactory
{
    public static BaseStyle CreateStyle(string styleType)
    {
        if (styleType == "point")
        {
            return CreatePointStyle();
        }
        if (styleType == "line")
        {
            return CreateLineStyle();
        }
        if (styleType == "poly")
        {
            return CreatePolytStyle();
        }
    }

    private static PointStyle CreatePointStyle()
    {
        //creates a pointstyle
    }

    private static LineStyle CreateLineStyle()
    {
        //creates a linestyle
    }

    private static PolyStyle CreatePolyStyle()
    {
        //creates a polystyle
    }
}

And then it's called in code like this:

PointStyle pointStyle = StyleFactory.CreateStyle("point");

Is this the best way to go about it? Should I split the three "Create" functions into their own separate class? Would using generics make more sense?

First: this kind of question is off-topic here (code that works, asking for subjective opinions). You might want to try at https://codereview.stackexchange.com/ If you want to post there be sure to read their help pages to make it on topic there. Second: the answer is: "It depends". For a generic advice I see no notable problem. It's ok. — bolov, Mar 11 '19 at 19:26
Why an extra step in execution path ... I mean an extra method `CreateLineStyle()`? — Rahul, Mar 11 '19 at 19:28
This is just a basic example. In reality there will be a few more parameters that will be passed to create the right style. — pfinferno, Mar 11 '19 at 19:29

John Wu · Accepted Answer · 2019-03-13T06:26:01.387

Think about the way the caller will have to use the method:

//Existing
BaseStyle someStyle = factory.CreateStyle("point", name);

There are two issues here. Once of them is that there is no compile-time evaluation of the string "point"; to mitigate this issue you might use a constant string or something similar. The second is that the returned object is just a BaseStyle; in order to do anything interesting, the client will always have to cast it. So in reality the code will look like this:

//Practical use of existing
PointStyle pointStyle = (PointStyle)factory.CreateStyle(Constants.StylesPoint, name);

We can address both of these issues with generics. If we define the method the right way, the return type is automatically chosen for us at compile time. This also means that the selection of type is checked at compile time, so we don't have to worry that the string was wrong. Example of what the call would look like:

//Using generics
PointStyle pointStyle = factory.CreateStyle<PointStyle>(name);

To allow the caller to use the method this way, we define a single type parameter:

public T CreateStyle<T>(string name) where T : BaseStyle
{       
    var type = typeof(T);
    if (type == typeof(PointStyle)) 
    {
        return new PointStyle(name) as T;
    }
    if (type == typeof(LineStyle)) 
    {
        return new LineStyle(name) as T;
    }
    if (type == typeof(PolyStyle) 
    {
        return new PolyStyle(name) as T;
    }
    throw new Exception("The type {0} is not supported.", typeof(T).FullName);
}

Or if you want to be clever about it:

public T CreateStyle<T>(string name) where T : BaseStyle
{       
    try
    {
        return Activator.CreateInstance(typeof(T), new [] { name } );
    }
    catch(MissingMethodException exception)
    {
        throw new InvalidOperationException("The specified style does not have an appropriate constructor to be created with this factory.", exception);      
    }    
}

This last approach requires no maintenance to add additional styles later; as long as they inherit from BaseStyle, and they contain a constructor that accepts the name as a single string argument, the factory will automatically be able to produce them.

Additional note:

While static factory methods were all the rage a few years ago, these days they are generally implemented as instance methods, so you can inject the factory under IoC. If you make the method static, any code that calls it will have a static dependency, which is difficult to stub and unit test.

Thank you for your answer. This works great. I've looked at IoC a bit, but I don't think it's right for my scenario. I had to convert a full app to a standalone control where all the objects created are strictly within this control. — pfinferno, Mar 12 '19 at 02:08
Glad to help. If you're writing controls. I'm in agreement that IoC is not appropriate. — John Wu, Mar 12 '19 at 04:46

Rahul · Answer 2 · 2019-03-11T19:43:57.257

1

I don't see any need for an extra abstraction method like CreatePolyStyle(); rather you can simply create the instance and return the same like

    public static BaseStyle CreateStyle(string styleType)
    {       
      BaseStyle style = null;    
   switch(styleType)
   {
        case "point":
            style = new PointStyle("abc");
            break; 
        case "line":
            style = new LineStyle("xyz");
            break; 
        case "poly":
           style = new PolyStyle("def");
           break;
        default:
           break;
   }
     return style;
    }

edited Mar 11 '19 at 19:43

answered Mar 11 '19 at 19:31

Rahul

76,197
13
71
125

I do like that better. Do you think there would be a point to make an interface and make the `CreateStyle` function generic instead of returning `BaseStyle`? – pfinferno Mar 11 '19 at 19:33
@pfinferno may be but I don't see an issue with current implementation ewither – Rahul Mar 11 '19 at 19:45

Correct implementation of factory

2 Answers2