Advice on "Factory" Pattern Implementation

Question

I'm refactoring some code around a couple of ASP.NET Web Forms pages and decided to try out a variation of the Abstract Factory pattern of my own design. I need to create an implementer of an abstract class based on what query string value is available.

Here's (a simplified version of) what I have right now:

internal static class Factory
{
 public static AbstractClass Create(NameValueCollection queryString)
 {
 if (queryString["class1"] != null)
 {
 return new Class1(queryString["class1"]);
 }
 if (queryString["class2"] != null)
 {
 return new Class2(queryString["class2"]);
 }
 if (queryString["class3"] != null)
 {
 return new Class3(queryString["class3"]);
 }
 return null;
 }
}

I don't know how I feel about this implementation. It certainly is better than what was there before, but I have a strong sense this could be improved some how.

So now, in the relevant pages, I can simply do this:

AbstractClass item = Factory.Create(Request.QueryString);
item.DoStuff();

Does anyone have any suggestions? Or would this be considered good as-is?

• Curtis Rutland: What is your desired behavior if your querystring looked something like this: ?class1=someVal&class2=someOtherVal? That's something to consider, since querystrings are client-facing and can be modified by them.
• Curtis Rutland: Also, do you need a separate Factory class? Why not make a static method on your Abstract class called Create? There are examples of this in the .NET Framework.
• Sven Grosen: If the client mucks with the query string, the desired behavior would be to simply go with the first value encountered in the order listed above
• Sven Grosen: And no, I don't need a separate Factory class, but my gut feel was to abstract that logic away from the base. That is a good suggestion though...
• Curtis Rutland: I feel that we can go too far with extracting functionality. Abstract classes allow for implemented methods, as well as static methods. It's worth grouping your logic together, IMO. I suppose my only other question is, how many potential classes do you have? A switch statement might be more efficient (cite)

Answer 1

Passing a query string, or any string really, to the factory is troubling.

Why I avoid strings:

1. typos
2. CasEinG
3. Presumed encoding/structure can be problematic.

IMHO the ideal will be to pass a parameter to the factory that unambiguously tells what to construct. As is, the factory is making gross assumptions about the content & validity of the incoming query string. But there is no telling what free-form text is in it.

The client code should evaluate & validate the query string and then pass the appropriate thing to the factory. And IMHO do not pass a string. Use enums. enums are:

1. type safe
2. avoid typos
3. self documenting - all the possible class-types are defined here.
4. Query string validation - now that we have a definitive list of all possible class-types we can verify the query string contents against the enum
5. intelisense in visual studio

Design thoughts

1. Below is not the Abstract Factory Pattern per se. We are not creating factories of factories. The point of a factory is to separate "complex construction (code)" from the using class. The more complex the constructions "the more factory we need".
2. Simple? then a static (or not) method w/in the client class is ok.
3. Longish & somewhat complex? a separate class.
4. Use the Factory in many places? a separate class might be best.
5. Many variations of each class-type? Perhaps a full-blown abstract factory design. etc., etc, and etc.
6. IMHO, making the Factory class an instance, static, or singleton is your call.

The Dictionary Examples

1. Other examples using a dictionary are interesting. I'd use an enum for the key, not strings. overall it feels like the reflection/abstraction aspects are wasted as it's all internal and you still have to "open the factory", i.e. modify it, to add/delete class-types.
2. The dictionary is necessary only because we pass the entire query string to the factory. Again, Tell the factory what to build don't make it try to guess (see above about "gross assumptions"). The calling client is the absolute best place with the proper context to decide what to build.
3. Making the factory parse the query string - and therefore handle problems - is violating the single responsibility principle.

.

internal public class Factory{
 public AbstractClass Create(abstracts makeThis) {
 AbstractClass newClass;
 switch(makeThis) {
 case abstracts.class1:
 newClass = BuildClass1();
 break;
 case abstracts.class2:
 newClass = BuildClass2();
 break;
 // and so on.
 default:
 throw new NotImplementedException();
 break;
 }
 return newClass;
 }
 // build methods here.
}
public enum abstracts {class1, class2, class3}
public class Client{
 AbstractClass factoryBuiltThing;
 Factory classFactory = new Factory();
 // put some error handling in here
 protected bool ParseQueryString(string queryString, out buildThis) {}
 // put some error handing here too.
 protected abstracts DeriveAbstractEnum(string theDesiredClass){};
 public void BuildSomething(string queryString) {
 string buildThis = string.Empty;
 abstracts whatToBuild;
 if (ParseQueryString (queryString, out buildThis)) {
 whatToBuild = DeriveAbstractEnum(buildThis);
 factoryBuiltThing = classFactory.Create(whatToBuild);
 }else { 
 throw new NotImplementedException(
 string.format("The query string is crap: {0}", queryString));
 }
 }
}

• Sven Grosen: I really like your point about the way I have it violating the single responsibility principle, excellent.

Answer 2

I Think you should do it more generic:

public static class Factory
{
 static Dictionary<String, Func<String, Abstract>> classes;
 public static Abstract GetInstance(String s){
 if(!classes.ContainsKey(s)) throw new Exception();
 Func<String, Abstract> create=classes[s];
 return create(s);
 }
 static Factory ()
 {
 classes=new Dictionary<String, Func<String, Abstract>>(){
 {"class1", (x)=>new Class1(x)},
 {"class2", (x)=> new Class2(x)}
 };
 }
}

This code is way more readable. You have the static constructor block, where you register the factory methods according to the keys. And the GetInstance-Method shrinks to 3 lines, of which one is a purely guarding clause.

Edit: modified after Mat's Mug's hint.

P.S: Oh, I forgot to mention, that you really never should return null.

Better a) throw an Exception or b) use a NullObject.

• Mathieu Guindon: +1 but I'd probably use a Collection Initializer to initialize classes in the static constructor (in the end it's identical).
• Sven Grosen: In your solution, where would the logic be to parse the query string value collection to figure out which class to initialize?
• Thomas Junk: Nowhere. That's not business of the factory. That should be done before. The factory delivers upon order - so to say. To make the order is not its duty ;)

Answer 3

I would implement it using reflection.

I'm using a Dictionary where the class types (inherited from AbstractClass) are stored, using the query string key as key. Also a List where I store keys, to keep track of sort order (the first inserted is the first checked for existence)

In the static constructor, the key and the type (for each class inherited from AbstractClass) are added to the mentioned data containers.

The Create method loops through all (sorted) keys contained in the _keys list. As soon as a corresponding key is found in the query string, the class type associated to that key is retrieved from the dictionary, and an instance is dynamically created, passing in the value associated to the key in the query string.

Although there is more code than your current implementation, I think it's more elegant and generic, and less error prone.

internal static class Factory
{
 private static readonly Dictionary<string, Type> _types = new Dictionary<string, Type>();
 private static readonly List<string> _keys = new List<string>();
 static Factory () {
 // Note: the order in which elements are inserted into the dictionary
 // determines which class has higher priority (in case multiple classes
 // are specified in the name value collection
 Add("class1", typeof(Class1));
 Add("class2", typeof(Class2));
 Add("class3", typeof(Class3));
 }
 private static void Add(string key, Type classType) {
 _types[key] = classType;
 _keys.Add(key); 
 }
 /// Use this if the key corresponds to the class name
 private static void Add(Type classType) {
 Add(classType.Name, classType);
 }
 public static AbstractClass Create(NameValueCollection queryString) {
 AbstractClass instance = null;
 for (int count = 0; count < _keys.Count; ++count) {
 var key = _keys[count];
 var queryStringValue = queryString[key];
 if (queryStringValue != null) {
 var classType = _types[key];
 var constructorParam = queryString[key];
 var parameters = new string[] {constructorParam};
 instance = (AbstractClass) Activator.CreateInstance(classType, parameters); 
 break;
 }
 }
 return instance;
 }
}

• Sven Grosen: Your for loop has a problem: ++count instead of count++. I don't know how I feel about using reflection for this, but thank you for the answer; very interesting.
• Antonio: ++count and count++ end up to the same result when used as standalone statements. I am used to the former because old unoptimized compilers translated count++ as: create a copy of count, increment it, then assign to count, whereas ++count was translated as an atomic inc assembler instruction.
• Sven Grosen: wow, you are right; i did not know that.