Am I overdoing it with my Factory Method?_问答_开发者

Part of our core product is a website CMS which makes use of various page widgets. These widgets are responsible for displaying content, listing products, handling event registration, etc. Each widget is represented by class which derives from the base widget class. When rendering a page the server grabs the page's widget from the database and then creates an instance of the correct class. The factory method right?

Private Function WidgetFactory(typeId)
    Dim oWidget
    Select Case typeId
        Case widgetType.ContentBlock
            Set oWidget = New ContentWidget
        Case widgetType.Registration
            Set oWidget = New RegistrationWidget
        Case widgetType.DocumentList
            Set oWidget = New DocumentListWidget
        Case widgetType.DocumentDisplay
    End Select
    Set WidgetFactory = oWidget
End Function

Anyways, this is all fine but as time has gone on the number of types of widgets has increased to around 50 meaning the factory method is rather long. Every time I create a new type of widget I go to add another couple of lines to the method and a little alarm rings in my head that maybe this isn't the best way to do things. I tend to just ignore that alarm but it's getting louder.

So, am I doing it wrong? Is there开发者_运维知识库 a better way to handle this scenario?

I think the question you should ask yourself is: Why am I using a Factory method here?

If the answer is "because of A", and A is a good reason, then continue doing it, even if it means some extra code. If the answer is "I don't know; because I've heard that you are supposed to do it this way?" then you should reconsider.

Let's go over the standard reasons for using factories. Here's what Wikipedia says about the Factory method pattern:

[...], it deals with the problem of creating objects (products) without specifying the exact class of object that will be created. The factory method design pattern handles this problem by defining a separate method for creating the objects, whose subclasses can then override to specify the derived type of product that will be created.

Since your WidgetFactory is Private, this is obviously not the reason why you use this pattern. What about the "Factory pattern" itself (independent of whether you implement it using a Factory method or an abstract class)? Again, Wikipedia says:

Use the factory pattern when:

The creation of the object precludes reuse without significantly duplicating code.

The creation of the object requires access to information or resources not appropriate to contain within the composing object.

The lifetime management of created objects needs to be centralised to ensure consistent behavior.

From your sample code, it does not look like any of this matches your need. So, the question (which only you can answer) is: (1) How likely is it that you will need the features of a centralized Factory for your widgets in the future and (2) how costly is it to change everything back to a Factory approach if you need it in the future? If both are low, you can safely drop the Factory method for the time being.

EDIT: Let me get back to your special case after this generic elaboration: Usually, it's a = new XyzWidget() vs. a = WidgetFactory.Create(WidgetType.Xyz). In your case, however, you have some (numeric?) typeId from a database. As Mark correctly wrote, you need to have this typeId -> className map somewhere.

So, in that case, the good reason for using a factory method could be: "I need some kind of huge ConvertWidgetTypeIdToClassName select-case-statement anyway, so using a factory method takes no additional code plus it provides the factory method advantages for free, if I should ever need them."

As an alternative, you could store the class name of the widget in the database (you probably already have some WidgetType table with primary key typeId anyway, right?) and create the class using reflection (if your language allows for this type of thing). This has a lot of advantages (e.g. you could drop in DLLs with new widgets and don't have to change your core CMS code) but also disadvantages (e.g. "magic string" in your database which is not checked at compile time; possible code injection, depending on who has access to that table).

The WidgetFactory method is really a mapping from a typeId enumeration to concrete classes. In general it's best if you can avoid enumerations entirely, but sometimes (particularly in web applications) you need to round-trip to an environment (e.g. the browser) that doesn't understand polymorphism and you need such measures.

Refactoring contains a pretty good explanation of why switch/select case statements are code smells, but that mainly addresses the case where you have many similar switches.

If your WidgetFactory method is the only place where you switch on that particular enum, I would say that you don't have to worry. You need to have that map somewhere.

As an alternative, you could define the map as a dictionary, but the amount of code lines wouldn't decrease significantly - you may be able to cut the lines of code in half, but the degree of complexity would stay equivalent.

Your application of the factory pattern is correct. You have information which dictates which of N types is created. A factory is what knows how to do that. (It is a little odd as a private method. I would expect it to be on an IWidgetFactory interface.)

Your implementation, though, tightly couples the implementation to the concrete types. If you instead mapped typeId -> widgetType, you could use Activator.CreateInstance(widgetType) to make the factory understand any widget type.

Now, you can define the mappings however you want: a simple dictionary, discovery (attributes/reflection), in the configuration file, etc. You have to know all the types in one place somewhere, but you also have the option to compose multiple sources.

The classic way of implementing a factory is not to use a giant switch or if-ladder, but instead to use a map which maps object type name to an object creation function. Apart from anything else, this allows the factory to be modified at run-time.

Whether it's proper or not, I've always believed that the time to use a Factory is when the decision of what object type to create will be based upon information that is not available until run-time.

You indicated in a followup comment that the widget type is stored in a database. Since your code does not know what objects will be created until run-time, I think that this is a perfectly valid use of the Factory pattern. By having the factory, you enable your program to defer the decision of which object type to use until the time when the decision can actually be made.

It's been my experience that Factories grow so their dependencies don't have to. If you see this mapping duplicating itself in other places then you have cause for worry.

try categories your widgets, maybe based on their functionality. if few of them are logically depending on each other, create them with single construction