patterncsharpMinor
Advice on "Factory" Pattern Implementation
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:
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:
Does anyone have any suggestions? Or would this be considered good as-is?
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:
This code is way more readable.
You have the static constructor block, where you register the factory methods according to the keys. And the
Edit: modified after Mat's Mug's hint.
P.S: Oh, I forgot to mention, that you really never should return
Better a) throw an Exception or b) use a
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.