HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Advice on "Factory" Pattern Implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
factoryimplementationadvicepattern

Problem

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?

Solution

I Think you should do it more generic:

public static class Factory
{
    static Dictionary> classes;

    public static Abstract GetInstance(String s){
        if(!classes.ContainsKey(s)) throw new Exception();
        Func create=classes[s];
        return create(s);
    }

    static Factory ()
    {
        classes=new Dictionary>(){
            {"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.

Code Snippets

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

Context

StackExchange Code Review Q#45013, answer score: 4

Revisions (0)

No revisions yet.