Factory pattern with controls

Question

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

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) };
                break;
            case WpfControl.HScrollBar:
                control = new WpfHScrollBar { InitialSize = new Size(130, 20), Orientation = Orientation.Horizontal };
                break;
            case WpfControl.VScrollBar:
                control = new WpfVScrollBar { InitialSize = new Size(20, 130), Orientation = Orientation.Horizontal };
                break;
            case WpfControl.Slider:
                control = new WpfSlider { InitialSize = new Size(130, 20) };
                break;
            case WpfControl.ProgressBar:
                control = new WpfProgressBar { InitialSize = new Size(130, 15), Value = 75};
                break;
            case WpfControl.ListView:
                var gridView = new GridView();
                gridView.Columns.Add(new GridViewColumn
                {
                    Header = "Items",
                });
                control = new WpfListView { InitialSize = new Size(100, 110), View = gridView };
                controlItems = CreateListItems();
                break;
            case WpfControl.TreeView:
                control = new WpfTreeView { InitialSize = new Size(150, 100) };
                controlItems = CreateTreeViewItems();
                break;
            case WpfControl.TabControl:
                control = new WpfTabControl { InitialSize = new Size(120, 80) };
                controlItems = CreateTabControlItems();
                break;
            case WpfControl.RichTextBox:
                control = new WpfRichTextBox { InitialSize = new Size(80, 80) };
                break;
            case WpfControl.DatePicker:
                control = new WpfDatePicker { InitialSize = new Size(130, 25) };
                break;
        }

        if (control != null)
        {
            control.Name = Consts.DefaultEaControlName;
        }

        var controlWithItems = control as IWpfControlWithItems;
        if (controlWithItems != null)
        {
            AddElementToItem(controlWithItems, controlItems);
        }

        return control;
    }

    private static void AddElementToItem(IWpfControlWithItems wpfControl, params object[] items)
    {
        foreach (var item in items)
        {
            wpfControl.Items.Add(item);
        }
    }

    private static object[] CreateTreeViewItems()
    {
        var rootExpandedNode = new TreeViewItem { Header = "Expanded Node", IsExpanded = true };
        var expandedNode = new TreeViewItem { Header = "Expanded Node", IsExpanded = true };
        var collapsedNode = new TreeViewItem { Header = "Collapsed Node", IsExpanded = false };
        var leaf1 = new TreeViewItem { Header = "Leaf" };
        var leaf2 = new TreeViewItem { Header = "Leaf" };
        var leaf3 = new TreeViewItem { Header = "Leaf" };

        expandedNode.Items.Add(leaf1);
        expandedNode.Items.Add(leaf2);
        rootExpandedNode.Items.Add(expandedNode);
        rootExpandedNode.Items.Add(collapsedNode);

        return new object[] { rootExpandedNode, leaf3};
    }

    private static object[] CreateTabControlItems()
    {
        return new object[]
                {
                    new TabItem { Header = "Tab1"} ,
                    new TabItem { Header = "Tab2" },
                    new TabItem { Header = "Tab3" }
                };
    }

    private static object[] CreateListItems()
    {
        return new object[] { "item1", "item2", "item3", "item4", "item5" };
    }
}

These controlItems are always the same and it never change so I don't pass it in constructor.

Answer 1

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:

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

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<WpfControl.Button>();

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