views:

2050

answers:

20

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.

+22  A: 

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.

Andrew Hare
"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" QFT
annakata
Exactly what he is doing? I don't see any subclasses decide here, just subclasses returned.
Peter
Maybe we're missing the point here, but I also don't see a subclassed interface.
Robert Massa
+14  A: 

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");
JaredPar
What is wrong with returning null?
Matthew Whited
I highly recommend this as it makes it much clearer when your extending an existing system and miss something.
JoshBerke
+1 For InvalidOperationException - C# is very forgiving of enum values and it would be good to know if someone passed in an invalid value cast as that enum type.
Andrew Hare
@Matt it really depends on the usage. But for me if I call a function in a factory pattern I fully expect it to 1) create an object or 2) fail and throw. Returning null is meaningless because it gives the caller no context on why the operation failed. True there are cases where null is valid and has meaning. This may be one of them. But I typically don't expect a null return from a factory pattern
JaredPar
Cool. yes, i definitely should add this.
If you are using conditional factories with more than just a fixed enum you then need to wrap with a tray catch instead of testing for null. I think as long as it is documented either way works. Maybe if you want to do throw an exception you may use assertions instead. This is one of those places where religious debates begin.
Matthew Whited
+4  A: 

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...

Marc Gravell
A: 

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

unforgiven3
+2  A: 

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.

matt b
Why do you think it isn't maintainable because he's using a switch statement? That seems perfectly maintainble to me.
unforgiven3
http://en.wikipedia.org/wiki/Factory_method_pattern shows a switch statement in an example. Seems fine to me.
unforgiven3
A switch is fine in simple cases in more complex cases it might become difficult to maintain; however, until you have the complex cases I'd stick with simple.
JoshBerke
I'd argue that the factory pattern is being used incorrectly if it becomes so complex that it is difficult to maintain by using a switch statement. I'd seriously consider some refactoring at that point.
unforgiven3
Personally I think switch statements should only be used when the fall through behavior is actually needed. Otherwise, it is too easy to forget the break statement.
Yishai
This is C#, you can't have implicit fall through behavior.I personally would have used an IDictionary<Enum, Func<T>>...
Min
I would typically use Dictionary if I wanted to actually invoke some action or function delegate. however, I am looking to instantiate some subclass, not invoke a method. I mean, sure, I can wrap the creations inside methods and use Dictionary to invoke using the type as a key. But the point of this was, is this a factory method?
+3  A: 

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.
        }
    }
}
Anton Gogolev
A: 

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.

JoshBerke
The basic answer would be: his tech lead is an idiot who is talking out of her ass.
TheTXI
TheTXI: Thank you for saying what I was thinking:-)
JoshBerke
Agreed with Josh. +1
unforgiven3
A: 

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

Grzenio
+7  A: 

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.

willcodejavaforfood
A: 

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.

Yishai
mmm , Head First is refered to quite a lot, and especially in none academic debates.
Peter
+3  A: 

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.)

j_random_hacker
A: 

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!!!

+6  A: 

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

Scott Lance
A: 

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.

LPalmer
Textbook? What book than? Not GoF or head first for sure.
Peter
+4  A: 

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.

corey broderick
I beleive that is the Abstract Factory pattern
willcodejavaforfood
http://en.wikipedia.org/wiki/Abstract_factory_pattern
A: 

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.

Aaron Daniels
+13  A: 

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.

Why is it that the right answers get so little votes?
Peter
+8  A: 
Ice09
What tool did you use to generate these diagrams? I like the sketchy look and feel.
Rob Sobers
That's yUML: http://yuml.me It's just awesome, you generate UML by text (eg. [Customer]+1->*[Order]) and the result is rendered as an image or PDF with a simple link like this: http://yuml.me/5f1fa3ba which you can include in any content, eg. <img src='http://yuml.me/5f1fa3ba'/>
Ice09
+5  A: 

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.

Peter
A: 

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;
    }
}
fabrizioM