Is this Factory Method creation pattern?

Question

I have been using factory method creation pattern for awhile now. I was just recently told that this:

public static class ScheduleTypeFactory
{
    public static IScheduleItem GetScheduleItem(ScheduleTypeEnum scheduleType)
    {
        IScheduleItem scheduleItem = null;

        switch (scheduleType)
        {
            case ScheduleTypeEnum.CableOnDemandScheduleTypeID:
                {
                    scheduleItem = new VODScheduleItem();
                    break;
                }
            case ScheduleTypeEnum.BroadbandScheduleTypeID:
                {
                    scheduleItem = new VODScheduleItem();
                    break;
                }
            case ScheduleTypeEnum.LinearCableScheduleTypeID:
                {
                    scheduleItem = new LinearScheduleItem();
                    break;
                }
            case ScheduleTypeEnum.MobileLinearScheduleTypeID:
                {
                    scheduleItem = new LinearScheduleItem();
                    break;
                }
        }

        return scheduleItem;
    }
}

is not a factory method creation pattern by my "Tech" lead without telling me why or giving me her interpretation. I kindly asked for an explanation and she told me she didn't have time. I was told to just rename it. If I am wrong, then I will no doubt accept that I have implemented this incorrectly for years. Is this how YOU would implement the factory method creation pattern? Thanks in advance.

Answer 1

Sure looks like the factory pattern to me. I don't see anything wrong with your implementation.

From Factory method pattern:

The essence of the Factory Pattern is to "Define an interface for creating an object, but let the subclasses decide which class to instantiate. The Factory method lets a class defer instantiation to subclasses."

This is exactly what you are doing.

As a side note: a good rule of thumb is that whenever someone tells you something and is unable or unwilling to provide a rationale for their statement, there is a good chance they are unqualified to make the statement at all.

Answer 2

Yes this is a factory pattern. My only comment would be that you fail silently for enum values that you don't specifically handle. That may be expected but I like to add the following to the end of statements like that

default:
  throw new InvalidOperationException("Invalid Enum Value");

Answer 3

Looks like a (basic) factory to me... in many factories the implementation is more complex (perhaps involving types resolved at runtime), but that is not (AFAIK) a requirement. The only other critique I'd have added is to combine the cases, and do something more dramatic if you don't recognise the type...

Answer 4

Yup, that's the factory pattern alright. Your Tech lead is wrong.

Answer 5

It is indeed a "factory" in that you have a method that returns a specific instance of the IScheduleItem based on some sort of logic; however, it probably isn't the best implementation or the most maintainable given that you are using a switch statement.

Answer 6

Well, Wikipedia says it is a factory method:

public class ImageReaderFactory 
{
    public static ImageReader getImageReader( InputStream is ) 
    {
        int imageType = figureOutImageType( is );

        switch( imageType ) 
        {
            case ImageReaderFactory.GIF:
                return new GifReader( is );
            case ImageReaderFactory.JPEG:
                return new JpegReader( is );
            // etc.
        }
    }
}

Answer 7

Looks like a basic factory pattern to me. I'm curious to hear why your tech lead doesn't think this is a pattern.

I'm also dissapointed she wouldn't take the time to explain things. Having been a tech lead before on a team, it is curcial to take the time to explain your decisions.

Answer 8

It is the factory method pattern, at least according to Wikipedia: http://en.wikipedia.org/wiki/Factory_method_pattern

Answer 9

I think it is traditionally called the simple factory pattern to distinguish it from the 'real' Abstract Factory pattern. It might be that you are not adhering to some sort of internal naming practice. She really ought to explain herself though.

Answer 10

What the tech lead is likely thinking is that the Factory pattern is to replace a constructor in the same object, not to return subclasses, or perhaps only return subclasses from a superclass, not from an unrelated object. It's wrong, but that is probably the thinking.

The fact that Wikipedia lists your pattern as an example shows that it is correctly a factory pattern. The patterns were defined to provide a common language for common solutions, so clearly if Wikipedia is showing this as an example, it is part of the common language. An academic debate about what the "Factory Pattern" is in some abstract sense misses the point of patterns.

Answer 11

That is the Factory pattern, but it's not necessarily the most maintainable variant. A more maintainable variant would maintain some sort of global map between ScheduleTypeEnum values and actual concrete subtypes of IScheduleItem -- that way, you could replace the switch statement with a lookup of the map.

Why is it more maintainable? Because subclass authors can add pairs to the map at the site where they derive the class, rather than in the GetScheduleItem() factory function itself. Hence the latter never needs updating; it is constantly up-to-date.

In C++ you can do this using a global std::map -- for each concrete subclass, the author of the subclass adds a dummy global variable which actually just registers the class (by adding to the map) in its constructor, which runs at program startup time. I'm certain that there's a convenient way to do the same thing in C#.

(C++ guru Herb Sutter has an entertaining description here, but it's fairly C++-heavy.)

Answer 12

Take back the power right now! Just drop 'Type' from the wording. ScheduleFactory FTW. Wait, is this a factory for 'Schedules' or 'ScheduleItems'? If it's scheduleitems then the factory should be called 'ScheduleItemFactory'. Be expressive!!!

Answer 13

Looks like a factory pattern to me. Tell your tech lead you don't have time to explain why she is wrong.

Answer 14

Thats a textbook Factory Pattern.

Some texts call it a Simple Factory Patteren, but I've never seen any criteria for what "Simple" means.

In my practice, a Factory Pattern is any intelligent creation of a concrete class coresponding to a desired interface.

But why fight your tech lead over naming a method. Rename it, move on with your life.

Answer 15

I'm surprised so many saying that this is the factory pattern. (So chances are that I'm thinking of this wrong, so please let me know.)

It looks to me like what you have there is only a part of the design. If you call it from your client, it's referred to as a "simple" factory, but it's not really considered a design pattern. (Don't get me wrong, I do this all the time).

The factory design pattern would state that your factory inherits/implements an abstract factory/factory interface.

Then, in your class which needs to use the factory (the client), you set the type of the factory to the abstract/interface, creating a concrete factory: i.e. --> IFactory factory = new ConcreteFactory();

The concrete factory would then create your IScheduleItem (leaving it to the factory to actually create the concrete type).

In the end I think the whole point is about loose coupling. While a "simple" factory loosely couples the construction of the product from the client, it does not decouple the factory. The factory pattern also decouples the factory.

Then again, it's early, I haven't had coffee, and I have a nasty habit of posting absolutely horrible responses that miss the entire point of the question.

Answer 16

The simplest explanation of the factory pattern, which I learned from a patterns and practice class, was that if you rely on another class to create the instances of your class, then you're using the factory pattern.

Of course, often times you want to add a layer of indirection by making your class abstract or an interface.

This is very simplified view of it, but either way, you are using the factory pattern.

Answer 17

I would agree to call the method a "Factory Method", though the design is not strictly a "Factory Method Pattern".
Here is a key point (from Wikipedia):

...The Factory method lets a class defer instantiation to subclasses."

Since your class is static and method static (hence non-virtual), there is no "deferring" possible.

Conceptually, notice also, that this implementation, though provides encapsulation, does not decouple/delay any decision.

Having said that, same Wikipedia article does present this schema as a variant of the "Factory Method Pattern".

Summary of the Summary: In my opinion this snippet is not a proper implementation of the "Factory Method OO Design Pattern", since it does not satisfy "a class defer instantiation to subclasses." Though, personally I would freely refer to this solution as "factory method".

To make it real factory method pattern, you need to allow the method to be overridden by subclasses. I.e. factory class (ScheduleTypeFactory) needs to be extensible (i.e. non-static), and GetScheduleItem needs to be virtual.

Answer 18

Your lead is right (sorry for the formatting, but this answer need some of it in order to be seen between the main line that is stating the lead is a moron): neither by the GOF book nor Head First this a factory method.

GoF :

“Define an interface for creating an object, but let the subclasses decide which class to instantiate. Factory Method lets a class defer instantiation to subclasses.”

In your example, it's not a subclass that is deciding.

Have you implemented it incorrectly all these years? No, you just haven't implemented the factory pattern, but what is sometimes referred to as the 'Simple Factory Pattern', which probably has done the job just fine.

Answer 19

Your Tech is right on renaming the method:

public static IScheduleItem GetScheduleItem(ScheduleTypeEnum scheduleType)

The action of the method is not to get something, is to create something. How do you decide which scheduleType should be created? Seems that logic should be encapsulated not the switch of the type.

Also why the static on the class? Where are you using it from?

public class ScheduleTypeFactory
{
    public static IScheduleItem createScheduleFrom(ScheduleTypeEnum scheduleType)
    {

        switch (scheduleType)
        {
            case ScheduleTypeEnum.CableOnDemandScheduleTypeID:
            case ScheduleTypeEnum.BroadbandScheduleTypeID:
                 return new VODScheduleItem();
             case ScheduleTypeEnum.LinearCableScheduleTypeID:
             case ScheduleTypeEnum.MobileLinearScheduleTypeID:
                    return new LinearScheduleItem();
        }

       raise InvalidSchedule;
    }
}