Home  /  Questions  /  Question



50   50
Mar 21, 2011


My first stab at the Factory Pattern. Feedback please.

I am building a staffing tool and using the Factory Pattern. Can you please take a look and let me know if I did it right? Also, I'm a bit confused about why I have to create a new creator class for each type. Can't I just make one method that returns the right employee type instead of making all of these creator classes?



// abstract product
public abstract class Employee  
{  
   public abstract void Add();  
   public abstract void Update();  
   public abstract Employee Get();  
   public abstract void Delete();  
   public abstract DataTable Search();  
   public string FirstName { get; set; }  
   public string LastName { get; set; }  
   public string Title { get; set; }  
}  

// concrete product 1
public class Developer : Employee  
{  
   public Developer()  
   {  
       Title = "Software Engineer";  
   }  
   public override void Add()  
   {  
   }  
   public override void Update()  
   {  
   }  
   public override Employee Get()  
   {  
       return new Model();  
   }  
   public override void Delete()  
   {  
   }  
   public override DataTable Search()  
   {  
       throw new NotImplementedException();  
   }  
}  


// concrete product 2
public class Secretary : Employee  
{  
   public Secretary()  
   {  
       Title = "Jr. Receptionist";  
   }  
   public override void Add()  
   {  
   }  

   public override void Update()  
   {  
   }  

   public override Employee Get()  
   {  
       return new Model();  
   }  

   public override void Delete()  
   {  
   }  

   public override DataTable Search()  
   {  
       throw new NotImplementedException();  
   }  
}  

// abstract creator
public abstract class EmployeeCreator  
{  
   // factory method
   public abstract Employee CreateEmployee();  
}  

// concrete creator 1
public class DeveloperCreator : EmployeeCreator  
{  
   public override Employee CreateEmployee()  
   {  
       return new Developer();  
   }  
}  

// concrete creator 2
public class SecretaryCreator : EmployeeCreator  
{  
   public override Employee CreateEmployee()  
   {  
       return new Secretary();  
   }  
}  

// console app to test this all
static void Main(string[] args)  
{  
    EmployeeCreator employeeCreator;  
    if (args.Length > 0)  
    {  
        EmployeeType type = (EmployeeType)Enum.Parse(typeof(EmployeeType), args[0], true);  
        if (type == EmployeeType.Secretary)  
        {  
            employeeCreator = new SecretaryCreator();  
        }  
        else
        {  
            employeeCreator = new DeveloperCreator();  
        }  
    }  
    else
    {  
        throw new InvalidOperationException();  
    }  
    Employee employee = employeeCreator.CreateEmployee();  
    Console.WriteLine(employee.Title);  
    Console.ReadLine();  
}  
 

 



50   50
Mar 21, 2011

Basically you are making the subclasses to decide what to instantiate.

If you need to set specific behavoiur or properties to the instance that is created in the creater class, then this is useful. Also it lets good separation of concerns.

For instance, if "Secretary" timings are different based on the location, then those things can be set in the creater class.

Hope this helps you to understand...


1,364   100.0
Mar 22, 2011
Hi Joe,

You are pretty right.  In fact rather than creating so many  creator you can use the c# language feature generics and create one creator which accepts a generic type. Your code would be more maintainable.

For sample implementation do check the following links..

http://aabs.wordpress.com/2007/11/21/a-generic-class-factory-for-c-30/


Short example here...(haven't tested this)

public class Program
    {
        // console app to test this all
        static void Main(string[] args)
        {
            EmployeeType type;

            if (args.Length > 0)
            {
                type = (EmployeeType)Enum.Parse(typeof(EmployeeType), args[0], true);
            }
            else
            {
                throw new InvalidOperationException();
            }

            Employee employee = EmployeeFactory<Employee>.Create(type);

            Console.WriteLine(employee.Title);
            Console.ReadLine();
        }
    }

    public static class EmployeeFactory<T> where T : Employee 
    {
           public static T Create(EmployeeType type) 
           {
               return (T)Activator.CreateInstance(Type.GetType("Test." + type.ToString()));            
           }
    }

NOTE:; This is a quick and dirty code.  In CreateInstance I have hardcoded the "namespace".  You may need to modify the code to remove this hardcoding. 

This code uses reflection.  Though reflection is costly but it depends of the nature of app you are building.  In most cases I found the benefit overriding the cost.

Let me know your thoughts. 



Hope this helps.




 3 comments
 
Thanks, Rajesh. But with the where constraint you have there, I would still have to create each EmployeeCreator class since you are stating that the Generic T parameter must implement EmployeeCreator. Would it be more correct to do something like: public Employee Create(EmployeeType type) { if(type == EmployeeType.Secretar) return new Secretary(); } ? --- Joe Rogers  Mar 22, 2011
 
But would using the suggestion I made not make this a factory pattern after all? --- Joe Rogers  Mar 22, 2011
 
Yeh.. In a hurry I have mixed two things.. You are right. I am updating the answer to reflect what I mean. What I intend to describe was the the factory method could be easily replaced by generics... --- Rajesh Pillai  Mar 22, 2011

50   50
Mar 23, 2011
Thanks, Rajesh. I made my actual implementation skip the generics and reflection. I'm not really sure if I would need them. If you find any fault with that statement, please let me know how I could benefit from using them instead of this method below. I put this method into my abstract employee class, fyi.



public static Employee EmployeeFactory(EmployeeType type)
{
             if (type == EmployeeType.Developer)
                return new Developer();
            else if (type == EmployeeType.Secretary)
                return new Secretary();

}

 
 1 comment
 
If this list (employeetype) is pretty fixed and limited then it is ok to have this here, otherwise you will find yourself modifying this piece of code everytime when a new EmployeeType needs to be added to the system and you will break the OCP (Open Closed Principle) :). In case of big systems this may have an impact on the testing cycles as well. Just my thoughts. But for small systems this may not be that big a change. But don't be intimidated by this. You can move ahead with your implementation and later revisit if you forsee any changes to this. --- Rajesh Pillai  Mar 24, 2011

1,128   99.9
Mar 24, 2011
I kinda like Rajesh version using generics. Here is another version of it (with generics)... what do you think?

namespace AbstractFactoryGenerics {
    using System;
    using System.Data;

    internal class Program {
        private static void Main() {
            var developer = EmployeeFactory.Create<Developer>();
            var secretary = EmployeeFactory.Create<Secretary>();

            Console.WriteLine(string.Format("I'm a {0}", developer));
            Console.WriteLine(string.Format("I'm a {0}", secretary));
        }

        public class Developer : Employee {
            public Developer() {
                Title = "Software Engineer";
            }

            public override void Add() {}
            public override void Update() {}
            public override Employee Get() {
                throw new NotImplementedException();
            }
            public override void Delete() {}
            public override DataTable Search() {
                throw new NotImplementedException();
            }

            public override string ToString() {
                return Title;
            }
        }

        public abstract class Employee {
            public string FirstName { get; set; }
            public string LastName { get; set; }
            public string Title { get; set; }
            public abstract void Add();
            public abstract void Update();
            public abstract Employee Get();
            public abstract void Delete();
            public abstract DataTable Search();
        }

        public class Secretary : Employee
        {
            public Secretary()
            {
                Title = "Jr. Receptionist";
            }
            public override void Add() { }

            public override void Update() { }

            public override Employee Get()
            {
                throw new NotImplementedException();
            }

            public override void Delete() { }

            public override DataTable Search()
            {
                throw new NotImplementedException();
            }

            public override string ToString()
            {
                return Title;
            }
        }

        public static class EmployeeFactory {
            public static T Create <T>() {
                return (T) Activator.CreateInstance(typeof(T));
            }
        }
    }
}

 
 5 comments
 
I think that using generics, as proposed by Rajesh, the code is easier to read. But that's subjective I guess :) --- Robert Blixt  Mar 24, 2011
 
Taking a second look at the generic implementation, Rajesh solution seems more flexible. --- Robert Blixt  Mar 24, 2011
 
Hi Robert,i can do this process create object in unity or another IOC containers. Why do we use factory pattern ,what is the difference between IOC container and FactoryPattern.I am searching 2 weeks, and second Joe wrote below code, does not it viaolate SOLID principles. Because using if/else statement. Can u help i am bit confused. Thanks public static Employee EmployeeFactory(EmployeeType type) { if (type == EmployeeType.Developer) return new Developer(); else if (type == EmployeeType.Secretary) return new Secretary(); } --- Volkan Genç  Mar 24, 2011
 
I too wonder sometimes if using a factory method negates the need for dependency injection and vice-versa? --- Joe Rogers  Mar 24, 2011
 
I consider DI to be a very configurable factory, take a look at this interesting thread for more info http://www.coderanch.com/t/427471/oa/Advantages-DI-over-factory-pattern --- Robert Blixt  Mar 25, 2011

50   50
Mar 26, 2011
Hi Joe,

I would prefer a single class called EmployeeCreatorFactory exposing a set of methods one for each employee type. Something like the following pseudo code:

public class EmployeeCreatorFactory
{
        public Employee createSecretary()
        {
        }

        public Employee createDeveloper()
        {
        }

        public Employee createContingentWorker()
        {
        }
}

Thanks.