patterncsharpMinor
Factory pattern with controls
Viewed 0 times
factorywithpatterncontrols
Problem
I've created factory to create WPF controls. I have string with control name and I map it to enum. I think it's bad implementation of factory pattern. So please show me where I've made mistake.
Main interface
Second interface
Factory class
```
public class WpfControlFactory
{
public static IWpfControl CreateWpfControl(WpfControl wpfControl)
{
object[] controlItems = {};
IWpfControl control = null;
switch (wpfControl)
{
case WpfControl.Button:
control = new WpfButton { InitialSize = new Size(85, 20) };
break;
case WpfControl.CheckBox:
control = new WpfCheckBox { InitialSize = new Size(85, 20) };
break;
case WpfControl.TextBox:
control = new WpfTextBox { InitialSize = new Size(75, 20) };
break;
case WpfControl.ComboBox:
break;
case WpfControl.ListBox:
control = new WpfListBox { InitialSize = new Size(100, 110) };
controlItems = CreateListItems();
break;
case WpfControl.GroupBox:
control = new WpfGroupBox { InitialSize = new Size(120, 100) };
break;
case WpfControl.RadioButton:
control = new WpfRadioButton { InitialSize = new Size(85, 20) };
break;
case WpfControl.TextBlock:
control = new WpfTextBlock { InitialSize = new Size(75, 20) };
break;
case WpfControl.Image:
control = new WpfImage { InitialSize = new Size(100, 100) };
Main interface
public interface IWpfControl
{
string Name { get; set; }
double ActualHeight { get; }
double ActualWidth { get; }
double Height { get; set; }
double Width { get; set; }
Size InitialSize { get; set; }
}Second interface
public interface IWpfControlWithItems : IWpfControl
{
ItemCollection Items { get; }
}Factory class
```
public class WpfControlFactory
{
public static IWpfControl CreateWpfControl(WpfControl wpfControl)
{
object[] controlItems = {};
IWpfControl control = null;
switch (wpfControl)
{
case WpfControl.Button:
control = new WpfButton { InitialSize = new Size(85, 20) };
break;
case WpfControl.CheckBox:
control = new WpfCheckBox { InitialSize = new Size(85, 20) };
break;
case WpfControl.TextBox:
control = new WpfTextBox { InitialSize = new Size(75, 20) };
break;
case WpfControl.ComboBox:
break;
case WpfControl.ListBox:
control = new WpfListBox { InitialSize = new Size(100, 110) };
controlItems = CreateListItems();
break;
case WpfControl.GroupBox:
control = new WpfGroupBox { InitialSize = new Size(120, 100) };
break;
case WpfControl.RadioButton:
control = new WpfRadioButton { InitialSize = new Size(85, 20) };
break;
case WpfControl.TextBlock:
control = new WpfTextBlock { InitialSize = new Size(75, 20) };
break;
case WpfControl.Image:
control = new WpfImage { InitialSize = new Size(100, 100) };
Solution
That is a Factory pattern, but it may not be the most efficient setup. If you had a finite set of controls, this wouldn't necessarily be bad, but here are some potential improvements.
Generics
They are your friend. Having the type enum is redundant and makes this class violate SRP (Single Responsibility Principle) because you will need to modify the factory any time you add a new type of control. You will also need to modify the enum, and create the class (three files modified for one change). If you remove the enum and resolve based on Type the method requires much less maintenance. And, by specifying a generic type constraint of IWpfControl, you can use the properties that they commonly share in your factory as well as limit the types of objects your factory can create.
For Example:
I also have an additional factory method that takes a
I made a reference to bone-headed exceptions in some comments above. This is in reference to an "Eric Lippert-ism" which you can read more about here.
Here is a quote from the article:
Boneheaded exceptions are your own darn fault, you could have
prevented them and therefore they are bugs in your code. You should
not catch them; doing so is hiding a bug in your code. Rather, you
should write your code so that the exception cannot possibly happen in
the first place, and therefore does not need to be caught. That
argument is null, that typecast is bad, that index is out of range,
you're trying to divide by zero – these are all problems that you
could have prevented very easily in the first place, so prevent the
mess in the first place rather than trying to clean it up.
As was pointed out by @craftworkgames, this may not be as clear-cut a case for the "bone-headed" exception as I made it out to be. Some responsibility should be on the user (developer) consuming the factory to not pass in invalid types (interfaces, abstract classes, etc...) and so you may want to just make the call to
Because you initialize some defaults on the new controls when they are created, we need to do some additional initialization, which leads me to the second point.
Constructor Initialization
There is no better authority on how to build an instance of an object than it's own constructor. If there are meaningful defaults that can be set on a new instance of a type, the constructor should specify them.
Instead of specifying the
This removes the need for this in the factory.
The only other point not addressed in my review (which I will try to address when I have more time) is what to do with the methods that build the list items based on the type of collection control. Often times, this is done with the use of an abstract factory pattern, where special types have their own factories for building them. I'll elaborate more on this when I get a bit more time.
For some information on how to implement this pattern, you can check this out:
http://www.dotnet-tricks.com/Tutorial/designpatterns/4EJN020613-Abstract-Factory-Design-Pattern---C
Generics
They are your friend. Having the type enum is redundant and makes this class violate SRP (Single Responsibility Principle) because you will need to modify the factory any time you add a new type of control. You will also need to modify the enum, and create the class (three files modified for one change). If you remove the enum and resolve based on Type the method requires much less maintenance. And, by specifying a generic type constraint of IWpfControl, you can use the properties that they commonly share in your factory as well as limit the types of objects your factory can create.
For Example:
public class WpfControlFactory
{
public static IWpfControl CreateWpfControl(Type controlType)
{
if(controlType == null)
{
throw new ArgumentNullException("controlType", "Cannot create a control. No type specified.");
}
IWpfControl wpfControl = null;
//Avoid some bone-headed exceptions
if(controlType.IsClass && !controlType.IsAbstract)
{
wpfControl = (IWpfControl)Activator.CreateInstance(controlType);
}
if(wpfControl != null)
{
wpfControl.Name = Consts.DefaultEaControlName;
if(wpfControl is IWpfControlWithItems)
{
var controlItems = CreateListItems();
var controlWithItems = wpfControl as IWpfControlWithItems;
AddElementToItem(controlWithItems, controlItems);
}
}
return wpfControl;
}
//Generic "overload" method using generics
public static TControl CreateWpfControl() where TControl : IWpfControl
{
return (TControl)CreateWpfControl(typeof(TControl));
}
}I also have an additional factory method that takes a
Type argument, this way you can use either to accomplish the same result. The invocation for each would look like this://When all you have is a type name
IWpfControl myControl = WpfControlFactory.CreateWpfControl(Type.GetType("WpfControl.Button"));
//Using Generics
WpfControl.Button myControl = WpfControlFactory.CreateWpfControl();I made a reference to bone-headed exceptions in some comments above. This is in reference to an "Eric Lippert-ism" which you can read more about here.
Here is a quote from the article:
Boneheaded exceptions are your own darn fault, you could have
prevented them and therefore they are bugs in your code. You should
not catch them; doing so is hiding a bug in your code. Rather, you
should write your code so that the exception cannot possibly happen in
the first place, and therefore does not need to be caught. That
argument is null, that typecast is bad, that index is out of range,
you're trying to divide by zero – these are all problems that you
could have prevented very easily in the first place, so prevent the
mess in the first place rather than trying to clean it up.
As was pointed out by @craftworkgames, this may not be as clear-cut a case for the "bone-headed" exception as I made it out to be. Some responsibility should be on the user (developer) consuming the factory to not pass in invalid types (interfaces, abstract classes, etc...) and so you may want to just make the call to
Activator.CreateInstance(controlType) and allow it to throw an exception, thus notifying the developer of their error.Because you initialize some defaults on the new controls when they are created, we need to do some additional initialization, which leads me to the second point.
Constructor Initialization
There is no better authority on how to build an instance of an object than it's own constructor. If there are meaningful defaults that can be set on a new instance of a type, the constructor should specify them.
Instead of specifying the
InitialSize property in your factory for each instance, just do so in the constructor of that control type.public class DatePicker : IWpfControl
{
public DatePicker()
{
this.InitialSize = new Size(130, 25);
}
}This removes the need for this in the factory.
The only other point not addressed in my review (which I will try to address when I have more time) is what to do with the methods that build the list items based on the type of collection control. Often times, this is done with the use of an abstract factory pattern, where special types have their own factories for building them. I'll elaborate more on this when I get a bit more time.
For some information on how to implement this pattern, you can check this out:
http://www.dotnet-tricks.com/Tutorial/designpatterns/4EJN020613-Abstract-Factory-Design-Pattern---C
Code Snippets
public class WpfControlFactory
{
public static IWpfControl CreateWpfControl(Type controlType)
{
if(controlType == null)
{
throw new ArgumentNullException("controlType", "Cannot create a control. No type specified.");
}
IWpfControl wpfControl = null;
//Avoid some bone-headed exceptions
if(controlType.IsClass && !controlType.IsAbstract)
{
wpfControl = (IWpfControl)Activator.CreateInstance(controlType);
}
if(wpfControl != null)
{
wpfControl.Name = Consts.DefaultEaControlName;
if(wpfControl is IWpfControlWithItems)
{
var controlItems = CreateListItems();
var controlWithItems = wpfControl as IWpfControlWithItems;
AddElementToItem(controlWithItems, controlItems);
}
}
return wpfControl;
}
//Generic "overload" method using generics
public static TControl CreateWpfControl<TControl>() where TControl : IWpfControl
{
return (TControl)CreateWpfControl(typeof(TControl));
}
}//When all you have is a type name
IWpfControl myControl = WpfControlFactory.CreateWpfControl(Type.GetType("WpfControl.Button"));
//Using Generics
WpfControl.Button myControl = WpfControlFactory.CreateWpfControl<WpfControl.Button>();public class DatePicker : IWpfControl
{
public DatePicker()
{
this.InitialSize = new Size(130, 25);
}
}Context
StackExchange Code Review Q#68559, answer score: 7
Revisions (0)
No revisions yet.