Is a switch statement applicable in a factory method? c#

Question

I want to return an Interface and inside a switch statement I would like to set it. Is this a bad design?

private IResultEntity GetEntity(char? someType)
    {
        IResultEntity entity = null;

        switch (someType)
        {
            case 'L': //life
                entity = new LifeEntity();
                break;
            case 'P': //property
                entity = new PropertyEntity();
                break;
            case 'D': //disability
                entity = new DisabilityEntity();
                break;
            case 'C': //credit card
                entity = new CreditCardEntity();
                break;
        }

        return entity;
    }

Answer 1

I wouldn't say its a bad design, although it is potentially fairly rigid. The only way to extend this would be via recompilation.

Answer 2

I dont know, which possibilities you have in c#, but it is still better to have one switch in a factory method than having switches all over the place. In a factory method, a switch is tolerable -- but better have it well documented.

Answer 3

I don't think there anything wrong with this. Yes, switch statements are a code smell, but in my book, they're OK in this sort of situation. There's actually little else you could do to achieve things like this.

Answer 4

I don't usually mind switch statements in a factory, provided I can group and control all of the derived classes that I want my factory to create in advance.

Sometimes,maybe a user-created plugin might want to add it's own classes to that switch list, and then a swich statement is not enough.

I found this good source for some more info on creating some more powerfull/versatile factory classes

A good middle-ground approach I usually take is to keep a static Dictionary< string,Type > for each factory class.

People can just "register" their own implementations using some sort of

Factories.TypeRegistration.StaticDictionary.Add("somekey",typeof(MyDerivedClass))

(or better yet, use a Registration method and hide the StaticDictionary)

then the Factory has an easy task of creating an instance by performing a lookup in the table:

Activator.CreateInstance(Factories.TypeRegistration.StaticDictionary["somekey"]);

Answer 5

I'd rather have the type you want to instantiate for a specific value in a config file. Something like:

<TypeMappings >
<TypeMapping name = "life" type ="Entities.LifeEntity,Entities"/ >
<TypeMapping name = "property" type ="Entities.PropertyEntity,Entities"/ >
<TypeMapping name = "disability" type ="Entities.DisabilityEntity,Entities"/ >
<TypeMapping name = "creditcard" type ="Entities.CreditCardEntity,Entities"/ >
</TypeMappings >

Inside your method you could then extract all the registrations from the config file, find the matching one and use reflection to instantiate the type, if registration is not found, you throw an exception.

Here is some sample code:

namespace Entities
{

public interface IResultEntity
{
}

public class LifeEntity : IResultEntity
{
 public override string ToString()
 {
  return("I'm a Life entity");
 }
}

public class PropertyEntity : IResultEntity
{
 public override string ToString()
 {
  return("I'm a Property Entity");
 }
}

public class CreditCardEntity : IResultEntity
{
 public override string ToString()
 {
  return("I'm a CreditCard Entity ");
 }
}

public class DisabilityEntity : IResultEntity
{
 public override string ToString()
 {
  return("I'm a Disability Entity");
 }
}

}

    public static Entities.IResultEntity GetEntity(string entityTypeName,string fileName)
{
    XDocument  doc = XDocument.Load(fileName);
 XElement element = doc.Element("TypeMappings").Elements("TypeMapping")
          .SingleOrDefault(x => x.Attribute("name").Value == entityTypeName);        

 if(element == null)
 {
  throw new InvalidOperationException("No type mapping found for " + entityTypeName);
 } 
 string typeName = element.Attribute("type").Value;
 Type type = Type.GetType(typeName);
 Entities.IResultEntity resultEntity = Activator.CreateInstance(type) as Entities.IResultEntity;
 if(resultEntity == null)
 {
  throw new InvalidOperationException("type mapping for " + entityTypeName +  " is invalid");
 }
 return resultEntity;
}

    public static void Main()
{
 try
 {
  Entities.IResultEntity result = GetEntity("life", @"c:\temp\entities.xml");
  Console.WriteLine(result);

   result = GetEntity("property", @"c:\temp\entities.xml");
  Console.WriteLine(result);

   result = GetEntity("disability", @"c:\temp\entities.xml");
  Console.WriteLine(result);   

   result = GetEntity("creditcard", @"c:\temp\entities.xml");
  Console.WriteLine(result);   

   result = GetEntity("foo", @"c:\temp\entities.xml");
  Console.WriteLine(result);  

 }
}

A lot of DI frameworks let you provide multiple registrations for an interface that you can query based on metadata. Check out this link on how MEF does exports using metadata.

Answer 6

It's not bad, it's almost exactly the same as an example (Parameterized Factory Method) in the Gang of Four Bible itself.

I used to think that switch statements are a code smell, they are not, they have their place in any OO language.