1

I read some tutorials about a the factory an abstract factory pattern and saw some examples of it. In one of the tutorials i read that the factory pattern could replace major "if" or "switch case" statements and follows the open/closed (solid) principles.

In one of my projects a have a huge "switch case" which i wanna replace by a(n) (abstract) factory. It's already interface based so implementing an factory shouldn't be that difficult but in all the examples i read in tutorials the factory produced a single concrete type based on configuration. Can anyone point me in the right direction how to implement a factory that could produce multiple types based on an enum that follows the Solid principles an replaces the large "switch case"....or am I misinformed and is the "switch case" moved to the factory?

code at this moment:

public interface ISingleMailProcessor : IMailProcessor
{
    MailResult ProcesMail(Mail mail);
}


public MailResult GetMailResult(mail)
{
  ISingleMailProcessor mailprocessor;
  switch (mail.MailType)
  {
      case MailConnector.MailType.AcronisBackup:
         mailprocessor = new AcronisProcessor();
         return mailprocessor.ProcesMail(mail);
      case ......etc.
  }
}
Luuk Krijnen
  • 1,180
  • 3
  • 14
  • 37

4 Answers4

1

In your case, just refactoring it into a method should be more than enough.

private ISingleMailProcessor CreateMailProcessor(MailType type)
{
  switch (type)
  {
      case MailConnector.MailType.AcronisBackup:
         return new AcronisProcessor();
      case ......etc.
  }
}

public MailResult GetMailResult(mail)
{
  ISingleMailProcessor mailprocessor = CreateMailProcessor(mail.MailType);
  return mailprocessor.ProcesMail(mail);
}

I don't think using factory is going to help you here. Factory would make sense if "type of mail" was decided outside the code that actually creates and sends the mail itself.

Then, the there would be specific factory for each type of mail to send, with interface to create a sender object. But even then, that would make sense if you needed a new instance of sender every time. In your case, just passing in the interface you have right now should be more than enough.

You can read my opinion on factories here : https://softwareengineering.stackexchange.com/a/253264/56655

Community
  • 1
  • 1
Euphoric
  • 12,645
  • 1
  • 30
  • 44
  • 1
    [Don't fall for the "YAGNI" excuse](http://programmers.stackexchange.com/questions/253254/why-do-people-nowadays-use-factory-classes-so-often/253264#comment509336_253264), the first comment mentions. The switch is already too big, as Luuk stated. – ntohl Jul 22 '15 at 09:05
  • I aggree the main problem i see that creating the instance is just a complete independent concern which should not be part of the business domain. No matter HOW it is extracted, it should be extracted – Boas Enkler Jul 22 '15 at 09:14
1

What you have there implemented is the Strategy pattern which is still a valid approach. Anyway, if you want to replace the whole switch and to make it more maintainable you can use this

public interface IProcessMail
{
     bool CanProcess(MailType type);
     MailResult GetMailResult(Mail mail);
}

Each mail processor will implement this interface. Then you'll have this

 public class MailProcessorExecutor
 {
     public MailProcessorSelector(IEnumerable<IProcessMail> processors)
     {
           _processors=processors;
     }

     public MailResult GetResult(Mail mail)
     {
         var proc=_processor.FirstOrDefault(p=>p.CanProcess(mail.MailType));
         if (proc==null)
         {
             //throw something
         }

         return proc.GetMailResult(mail);
     }

    static IProcessMail[] _procCache=new IProcessMail[0];

     public static void AutoScanForProcessors(Assembly[] asms)
     {
       _procCache= asms.SelectMany(a=>a.GetTypesDerivedFrom<IProcessMail>()).Select(t=>Activator.CreateInstance(t)).Cast<IProcessMail>().ToArray();
     }

     public static MailProcessorExecutor CreateInstance()
     {
        return new MailProcessorExecutor(_procCache);
     }
  }

  //in startup/config 
  MailProcessorExecutor.AutoScanForProcessors([assembly containing the concrete types]);

 //usage
 var mailProc=MailProcessorExecutor.CreateInstance();
var result=mailProc.GetResult(mail);

Ok, so, the point is there will be an object in charge of selecting and executing the processors. The static methods are optional, the AutoScan method searches any given assemblies for concrete IPocessMail implementations then instantiates them. This allows you to add/remove any processor and it will be used automatically (no other setup required). Also there is no switch involved and there will never be. Note that GetTypesFromDerived is a helper method I use (it's part of my CavemanTools library) and the Activator requires a parameterless constructor. Instead of it you can use a DI Container to get the instances (if you're using one or the processors have deps)

If you're using a DI Container, you don't need the CreateInstance method. Just register MailProcessorExecutor as a singleton and inject it (pass it as a constructor argument) where you need it.

MikeSW
  • 16,140
  • 3
  • 39
  • 53
  • You mean my approach used Strategy? Does the `GetTypesDerivedFrom` use reflection (You have to consider efficiency, and type safety)? – ntohl Jul 21 '15 at 15:02
  • I meant the OP code resembles Strategy. Yes `GetTypesDerivedFrom` uses reflection. Efficiency? :))) You have no idea how many things are using reflection under the cover. About type safety, yes, I forgot a cast (I've updated my answer to include it). – MikeSW Jul 21 '15 at 15:28
  • I tried to think about a Strategy solution for the OP's question, but there would be a switch, where the Strategy's context is assigned to the actually used `AcronisBackupStrategy`. Then call a `GetResult` on it. I think strategy is not applicable. I don't know how much reflection is used under the cover, but I had a project, where just eliminating 1 LINQ restored the usability of the software. – ntohl Jul 21 '15 at 15:34
  • Yes, the OP code had a switch in it and **his** code is Strategy. Mine is a combination. About your 1 Linq that was your specific edge case. Btw, only some parts of reflection are slow and what I've used isn't. – MikeSW Jul 21 '15 at 15:41
  • Not yet tried it but seams a legit solution for my problem. I'm gonna try it! – Luuk Krijnen Jul 23 '15 at 06:34
  • Thanks, worked fine! I only replaced the GetDerivedFrom method with one answered by @DarrenKopp in the following question: http://stackoverflow.com/questions/26733/getting-all-types-that-implement-an-interface – Luuk Krijnen Jul 24 '15 at 06:03
  • Downvoter, care to comment? Maybe you have a better solution. – MikeSW Jul 29 '15 at 16:02
0

First of all I want to recommand the CCD Webcast about factories. It is very helpfull on this topic and also shows some problems we may encounter. Also you can find some good informations on this objectmentor document

As a summarization of the webcast you could see, the more you want to follow the OpenClosed Principal in the problem domain of "creation" the less typesafe you can work.

As a result the abstract factory could also operate on a some more dynamic values like strings. For example you could have an Method.

string GetAllPossibilities(); // Returns all possible kinds that can be constructed

and a related

T Create<T>(string kind)

the call would now only have to pass the string which uniquely identifies the requested instance kind . Your marker could be something "selfmade" like "Rectangle" or event TypeNames, but that would mean that there is more depedency between the component, as a Namespace change could break the caller.

So your call may be something like one of these:

Factory.Create("Acronis") // Your Marker

Factory.Create("MYNameSpace.AcronisProcessor")  //TypeName

Factory.Create<AcronisProcessor>() //Type 

So you wouldn't have switch statements outside the Factory. Inside the factory you may have some or you could think about some kind of dynamic object creation.

The Factory could still have swithc statements switching your selfmade identifier or code do something like

var type = Type.GetType(kind);
return Activator.CreateInstance(type) as T;

But as this is seperated from the main domain logik it doesn't have anymore this importance like before-.

With this at first look you wouldn't have api realted breaking changes if you get new options.

But there is still some kind of underlying semantic dependency.

EDIT: As you can see in the discussion below i cut off some details as I think they would blure up the main point (OpenClosed Principales and Factory Pattern). But there are still some other Points that should not be forgotten. Like "What is an application root and how has it to be designed" . To get all details the webcast (also the others of this site too) are a much better approach to learn this details then this post here.

Boas Enkler
  • 12,264
  • 16
  • 69
  • 143
  • 1
    The answer is not even mentioning where are the switch cases in the example. In Your example, the `Create` will contain the switch case depending on `kind`. – ntohl Jul 21 '15 at 08:10
  • I just notices some minutes ago and i was right now extending my post to clearify it – Boas Enkler Jul 21 '15 at 08:13
  • IMHO it is not a good usage of factory. You have linked an Uncle Bob article, and U. Bob said that the switches should be contained in the Main method. And just for initialize the factories. You don't pass this parameter to create, but rather initialize the factory with the right type. – ntohl Jul 21 '15 at 08:16
  • Good point but imo it really depends on the problem domain. Surely i would generaly try to avoid the switch at all, but i cant see how this is in luuk domain. It would also be possibly to give the Factory a IEnumerable of allowed types or IENumerable> but this is very detailed and imo not so important as making the principle more clear. For example if he curretnly only has two options then imo a if statement would be enough to start. (Be aware of optimizations and YAGNI). The main point is that the domain doesn't get in touch with the application root – Boas Enkler Jul 21 '15 at 08:18
  • It wasn't me who downvoted, just for record. We can't consider YAGNI, because the need is already there. The switch is too big. – ntohl Jul 21 '15 at 08:34
  • no problem : ) hmm i can't see how big the switch is . Also his question was focused on how to implement the factory pattern looking at the openclosed principle :) but i agree with you on the technical content :) – Boas Enkler Jul 21 '15 at 08:45
0

You will need Visitor pattern for that.

{
    public MailResult GetMailResult(mail)
    {
        _objectStructure.Visit(mail)
    }
    ObjectStructure _objectStructure= new ObjectStructure();
    constructor() {
         _objectStructure.Attach(new AcronisBackupVisitor());
         _objectStructure.Attach(new ...)
    }
}

class AcronisBackupVisitor: Visitor {
    public override void Visit(HereComesAConcreteTypeDerivedFromMail concreteElement)
    {
         // do stuff
    }
    public override void Visit(HereComesAConcreteTypeDerivedFromMailOther concreteElement)
    {
        //don't do stuff. We are not in the right concrete mail type
    }
}

In this way, we can differentiate by the dynamic type of the concrete Mail you get. Just Make Mail abstract, and derive Acronis mail, and other types of mails from that. I have began the implementation of this example from here.

ntohl
  • 2,067
  • 1
  • 28
  • 32