Factory class knows too much

Question

UPDATED I've updated the example to better illustrate my problem. I realised it was missing one specific point - namely the fact that the CreateLabel() method always takes a label type so the factory can decide what type of label to create. Thing is, it might need to obtain more or less information depending on what type of label it wants to return.

I have a factory class that returns objects representing labels to be sent to a printer.

The factory class looks like this:

public class LargeLabel : ILabel
{
    public string TrackingReference { get; private set; }

    public LargeLabel(string trackingReference)
    {
        TrackingReference = trackingReference;
    }
}

public class SmallLabel : ILabel
{
    public string TrackingReference { get; private set; }

    public SmallLabel(string trackingReference)
    {
        TrackingReference = trackingReference;
    }
}

public class LabelFactory
{
    public ILabel CreateLabel(LabelType labelType, string trackingReference)
    {
        switch (labelType)
        {
            case LabelType.Small:
                return new SmallLabel(trackingReference);
            case LabelType.Large:
                return new LargeLabel(trackingReference);
        }
    }
}

Say that I create a new label type, called CustomLabel. I want to return this from the factory, but it needs some additional data:

public class CustomLabel : ILabel
{
    public string TrackingReference { get; private set; }
    public string CustomText { get; private set; }

    public CustomLabel(string trackingReference, string customText)
    {
        TrackingReference = trackingReference;
        CustomText = customText;
    }
}

This means my factory method has to change:

public class LabelFactory
{
    public ILabel CreateLabel(LabelType labelType, string trackingReference, string customText)
    {
        switch (labelType)
        {
            case LabelType.Small:
                return new SmallLabel(trackingReference);
            case LabelType.Large:
                return new LargeLabel(trackingReference);
            case LabelType.Custom:
                return new CustomLabel(trackingReference, customText);
        }
    }
}

I don't like this because the factory now needs to cater for the lowest common denominator, but at the same time the CustomLabel class needs to get a custom text value. I could provide the additional factory method as an override, but I want to enforce the fact that the CustomLabel needs the value, otherwise it'll only ever be given empty strings.

What is the correct way to implement this scenario?

Answer 1

This is probably an indication that a factory pattern isn't the best for you. If you do either need or wish to stick with it, though, I would suggest creating initialization classes/structs that can be passed into the factory, rather than the string. Whether you want to do it with various subclasses of a basic information class (basically creating an initialization class hierarchy that mimics that of your label classes) or one class that contains all of the information is up to you.

Answer 2

Well, how do you want to call the factory method?

Concentrate on how you want to be able to use your API, and the implementation will usually make itself fairly clear. This is made even easier if you write the desired results of your API as unit tests.

An overload may well be the right thing to do here, but it really depends on how you want to use the factory.

Answer 3

You should try to use a configuration class and pass an instance of that to the factory. The configuration classes would build a hierarchy, where a special configuration class would exist for each result you expect from the factory. Each configuration class captures the specific properties of the factory result.

For the example you've given I'd write a BasicLabelConfiguration and a CustomLabelConfiguration derived from it. The BasicLabelConfiguration captures the tracking reference, while the CustomLabelConfiguration captures the custom text.

Finally the factory makes a decision based on the type of the passed configuration object.

---

Here's an example of the code:

public class BasicLabelConfiguration
{
  public BasicLabelConfiguration()
  {
  }

  public string TrackingReference { get; set; }
}

public class CustomLabelConfiguration : BasicLabelConfiguration
{
  public CustomLabelConfiguration()
  {
  }

  public string CustomText { get; set; }
}

public class LabelFactory
{
  public ILabel CreateLabel(BasicLabelConfiguration configuration)
  {
    // Possibly make decision from configuration
    CustomLabelConfiguration clc = configuration as CustomLabelConfiguration;
    if (clc != null)
    {
      return new CustomLabel(clc.TrackingReference, clc.CustomText);
    }
    else
    {
      return new BasicLabel(configuration.TrackingReference);
    }
  }
}

Finally you'd use the factory like this:

// Create basic label
ILabel label = factory.CreateLabel(new BasicLabelConfiguration 
{
  TrackingReference = "the reference"
});

or

// Create basic label
ILabel label = factory.CreateLabel(new CustomLabelConfiguration 
{
  TrackingReference = "the reference",
  CustomText = "The custom text"
});

Answer 4

Without further information it's pretty hard to give any advice, but assuming that the factory pattern is what you actually need you could try the following approach:

Pack the needed arguments in some kind of property map (e.g. map of string to string) and pass that as an argument to the factory's create method. Use well-known tags as keys in the map, allowing the specialized factories to extract and interpret the mapped values to their own liking.

This will at least allow you to maintain a single factory interface for the time being, and postpone dealing with architectural issues if (or when) you notice that the factory pattern isn't the correct one here.

(Oh, and if you really want to use the factory pattern here I strongly suggest you make it pluggable to avoid having to modify the factory for each new label type).

Answer 5

How about just using the Factory method to decide what label you need?

public class LabelFactory {
    public ILabel CreateLabel(string trackingReference, string customText) {
        return new CustomLabel(trackingReference, customText);
    }

    public ILabel CreateLabel(String trackingReference) {
        return new BasicLabel(trackingReference);
    }
}

Your factory still needs to know about each type (although with an interface you can implement dynamic loading) but there is very little that the client needs to know - according to what data is provided, the factory generates the correct implementation.

This is a simplistic solution to the simple problem you described. I assume the question is an oversimplification of a more complex problem but without knowing what your real problem is, I'd rather not design an over complex solution.

Answer 6

You are trying to force the pattern into a scenario in which it does not fit. I would suggest giving up on that particular pattern and focus instead of making the simplest solution possible.

I think in this case I would just have one class, Label, that has a text field for custom text that is normally null/empty but which one can set if the label needs to be custom. It is simple, self-explanatory and will not give your maintenance programmers any nightmares.

public class Label
{
    public Label(string trackingReference) : this(trackingReference, string.Empty)
    {
    }

    public Label(string trackingReference, string customText)
    {
        CustomText = customText;
    }

    public string CustomText ( get; private set; }

    public bool IsCustom
    {
        get
        {
            return !string.IsNullOrEmpty(CustomText);
        }
    }
}

Answer 7

From reading your question it sounds like your UI collects the information and then uses the factory to create the appropriate label. We use a different approach in the CAD/CAM application I develop.

During startup my applications uses the factory method to create a master list of labels. Some of my labels have initialization parameters because they are variants of each other. For example we have three type of flat part labels. While others have parameters that are user defined or not known at setup.

In the first case the initialization is handled within the factory method. So I create three instances of FlatPartLabel passing in the needed parameters.

In the second case Label interface has a configure option. This is called by the label printer dialog to populate a setup panel. In your case this is where the tracking reference and CustomText would be passed in.

My label interface also returns a unique ID for each Label type. If I had a specific command to deal with that type of label then I would traverse the list of labels in my application find which one matches the ID, cast it to the specific type of label, and then configure it. We do this when user want to print one label only for a specific flat part.

Doing this means you can be arbitrary complex in the parameters your labels need and not burden your Factory with unessential parameters.

Answer 8

ANSWER UPDATED FOLLOWING UPDATE OF THE QUESTION - SEE BELOW

I still think you're right to be using the Factory pattern, and correct in overloading the CreateLabel method; but I think in passing the LabelType to the CreateLabel method, you're missing the point of using the Factory pattern.

Key point: the entire purpose of the Factory pattern is to encapsulate the logic which chooses which concrete subclass to instantiate and return. The calling code should not be telling the Factory which type to instantiate. The benefit is that the code which calls the Factory is therefore shielded from changes to that logic in the future, and also from the addition of new concrete subclasses to the factory. All your calling code need depend on is the Factory, and the Interface type returned from CreateLabel.

The logic in your code at the point where you call the Factory must currently look something like this pseudocode...

// Need to create a label now
ILabel label;
if(we need to create a small label)
{
   label = factory.CreateLabel(LabelType.SmallLabel, "ref1");
}
else if(we need to create a large label)
{
   label = factory.CreateLabel(LabelType.LargeLabel, "ref1");
}
else if(we need to create a custom label)
{
   label = factory.CreateLabel(LabelType.CustomLabel, "ref1", "Custom text")
}

...so you're explicitly telling the Factory what to create. This is bad, because every time a new label type is added to the system, you'll need to...

1. Change the factory code to deal with the new LabelType value
2. Go and add a new else-if to everywhere that the factory's called

However, if you move the logic that chooses the LabelType value into your factory, you avoid this. The logic is encapsulated in the factory along with everything else. If a new type of label is added to your system, you only need to change the Factory. All existing code calling the Factory remains the same, no breaking changes.

What is the piece of data that your current calling code uses to decide whether a big label or small label is needed? That piece of data should be passed to the factory's CreateLabel() methods.

Your Factory and label classes could look like this...

// Unchanged
public class BasicLabel: ILabel
{
    public LabelSize Size {get; private set}
    public string TrackingReference { get; private set; }

    public SmallLabel(LabelSize size, string trackingReference)
    {
        Size = size;
        TrackingReference = trackingReference;
    }
}



// ADDED THE NULL OR EMPTY CHECK
public class CustomLabel : ILabel
{
    public string TrackingReference { get; private set; }
    public string CustomText { get; private set; }

    public CustomLabel(string trackingReference, string customText)
    {
        TrackingReference = trackingReference;
        if(customText.IsNullOrEmpty()){
           throw new SomeException();
        }
        CustomText = customText;
    }
}



public class LabelFactory
{
    public ILabel CreateLabel(string trackingReference, LabelSize labelSize)
    {
         return new BasicLabel(labelSize, trackingReference);
    }

    public ILabel CreateLabel(string trackingReference, string customText)
    {
         return new CustomLabel(trackingReference, customText);
    }
}

I hope this is helpful.