2

I'm refactoring a legacy codebase.
I have 4 very similar objects that I decided would be a good target for becoming polymorphic, so I moved all the common code to a base class and added an interface.

The real problem arises when trying to create those objects. Every class has 3 different constructors with 5-10 parameters each, used in different cases based on the context.
I already know those classes have too much going on, and would actually be better split in different, smaller classes, each one dealing with one case, however this is a legacy codebase and I can't do so many changes to the existing code.

At the moment my code is something like this:

public class HandlerFactory
{
    public static IHandler CreateInstance(HandlerType param1, string param2, ..., string param8)
    {
        switch(param1)
        {
            case HandlerType.Type1
                return new Handler1(param2, ..., param8)
            case HandlerType.Type2
                return new Handler2(param2, ..., param8)
            case HandlerType.Type3
                return new Handler3(param2, ..., param8)
            case HandlerType.Type4
                return new Handler4(param2, ..., param8)
        }
    }

    public static IHandler CreateInstance(HandlerType param1, int param2, ..., DateTime param10)
    {
        switch(param1)
        {
            case HandlerType.Type1
                return new Handler1(param2, ..., param10)
            case HandlerType.Type2
                return new Handler2(param2, ..., param10)
            case HandlerType.Type3
                return new Handler3(param2, ..., param10)
            case HandlerType.Type4
                return new Handler4(param2, ..., param10)
        }
    }

    public static IHandler CreateInstance(HandlerType param1, string param2, ..., CustomClass param5)
    {
        switch(param1)
        {
            case HandlerType.Type1
                return new Handler1(param2, ..., param5)
            case HandlerType.Type2
                return new Handler2(param2, ..., param5)
            case HandlerType.Type3
                return new Handler3(param2, ..., param5)
            case HandlerType.Type4
                return new Handler4(param2, ..., param5)
        }
    }
}

However, I hate having 3 factory methods, as I now have to edit three of them if I were to add another Handler (the main reason I'm doing this refactoring).

Is there a way I can keep just one factory method with a single switch?

  • What is strange here is that you have three different parametrized constructors. Those are typically used to initialize some attributes of a class using the arguments passed through constructors. If you have one constructor with four parameters, and another with 9, does that mean that five attributes remain uninitialized, or are defaulted in some way in the former case? Or, is this simply the case where you transform the types in some way within constructors, e.g. DateTime in one constructor, and three integers representing day, month and year in another? – Vladimir Stokic Apr 7 '17 at 10:45
  • @VladimirStokic it's much much worse. Every handler creates different files (with different meanings altogether) based on the constructor called. As I said, it would really be better as multiple distinct classes (one per constructor), but the code inside is a real mess and I can't rework eveything now – BgrWorker Apr 7 '17 at 11:47
2

The answer provided by @JohnWu is very elegant and I like it a lot. Allow me to provide an alternative.

You could use Abstract Factory Pattern. You would basically have one big factory of factories, which would then produce the concrete factories for concrete handlers. Then, should you need another handler, all you would need to do is write one factory for that particular handler. It would be something along these lines:

public interface IHandler
{
}

public class Handler1 : IHandler
{
    public Handler1(string param1, string param2)
    {
    }

    public Handler1(int param1, int param2)
    {
    }
}

public class Handler2 : IHandler
{
    public Handler2(string param1, string param2)
    {
    }

    public Handler2(int param1, int param2)
    {
    }
}

For simplicity, I have only two Handler classes with two different constructors. I believe the solution still can be applied to your case. Next, we have factories:

   public interface IHandlerFactory
    {
        IHandler Create(string param1, string param2);
        IHandler Create(int param1, int param2);
    }

    public class Handler1Factory : IHandlerFactory
    {
        public IHandler Create(int param1, int param2)
        {
            return new Handler1(param1, param2);
        }

        public IHandler Create(string param1, string param2)
        {
            return new Handler1(param1, param2);
        }
    }

    public class Handler2Factory : IHandlerFactory
    {
        public IHandler Create(int param1, int param2)
        {
            return new Handler2(param1, param2);
        }

        public IHandler Create(string param1, string param2)
        {
            return new Handler2(param1, param2);
        }
    }

    public class HandlerFactory
    {
        public static IHandler Create(IHandlerFactory factory, string param1, string param2)
        {
            return factory.Create(param1, param2);
        }

        public static IHandler Create(IHandlerFactory factory, int param1, int param2)
        {
            return factory.Create(param1, param2);
        }
    }

The alternative here would be to have a HandlerFactory class that would have a property of IHandlerFactory type, and the methods would not be static and would not have IHandlerFactory parameter in them. Obviously, an object of HandlerFactory class would need to be instantiated at some point.

The usage would go somehow like this:

public class Example
{
    public void ExampleMethod()
    {
        Handler1Factory factory1 = new Handler1Factory();
        var handler1 = HandlerFactory.Create(factory1, "parameter1", "parameter2");

        Handler2Factory factory2 = new Handler2Factory();
        var handler2 = HandlerFactory.Create(factory2, 1, 2);
    }
}

The solution clearly divides the code by purpose, and most importantly, should you have the need for another handler, you would not need to change the existing code. All you would have to do is write a factory for that particular handler.

Remember, this is only possible because all handlers have the constructors with same parameter lists. The design would be somewhat different if that was not the case.

  • Why not use generics and constraints to avoid all of the ugly Java style casting. interface IHandlerFactory<T> where T: IHandler { T Create(string a, string b); } and then class Handler2Factory: IHandlerFactory<Handler2> { public Handler2 Create(string a, string b) => new Handler2(a, b); } – Aluan Haddad Apr 10 '17 at 4:27
  • Then add static class HandlerFactory { static THandler Create<THandler>(IHandlerFactory<THandler> factory, string param1, string param2) where THandler: IHandler => factory.Create(param1, param2); } and use as var handler2 = handlerFactory.Create(new Handler2Factory(), "a", "b"); – Aluan Haddad Apr 10 '17 at 4:31
  • You could absolutely do that to avoid explicit casting, but that is the only real benefit, and it makes the code slightly less legible. Since the people who work on the code are not quite adept to polymorphism, I assumed they are not too familiar with generics either, so I tried to write code that was as legible as possible that would address the primary issue the question was trying to tackle: how to avoid multiple switches, which essentially could be translated as: how to adhere to Open/Close Principle in this case. – Vladimir Stokic Apr 10 '17 at 6:22
  • 1
    @AluanHaddad I'd really like to read a full answer from you. Anyway, I don't really need casting to the concrete type. Interfaces are there for a reason. – BgrWorker Apr 10 '17 at 7:24
  • 1
    @AluanHaddad I used var in the example method, thus eliminating the need for type casting. Thanks for the advice. I too would like to read a full answer from you. – Vladimir Stokic Apr 10 '17 at 7:53
7

This solution eliminates the case/switch and replaces it with a generic argument. Normally you can't create objects this way unless you use the new constraint, which doesn't allow any constructor arguments; I stole this workaround from another answer.

Also, there is only one factory method here, and constructor arguments are accepted as a variable-sized list using the params keyword.

public class HandlerFactory
{
    public static T CreateInstance<T>(params object[] constructorArguments) where T : class, IHandler
    {
        return (T)Activator.CreateInstance(typeof(T), constructorArguments);
    }
}

As you can see, this method is much shorter, has no case/switch, and only requires one prototype. And the nice thing is if you ever add Handler4 or any other subclass, you don't need to touch the factory. Just make sure that Handler4 implements IHandler and you're already all set.

You can call it like this:

var o = HandlerFactory.CreateInstance<Handler3>(param1, param2);

Because the return type is T you have your choice of declaring o as a var (and the compiler will infer the concrete type) or you could use

IHandler o = HandlerFactory.CreateInstance<Handler3>(param1, param2);

if you prefer to only work with the interface.

  • This would be perfect, however my team hardly understands polymorphism (hence the code mess), so if I use reflection I fear their head might explode. If no better answer shows up I think I'll use this method, and heavily document its functionality. – BgrWorker Apr 7 '17 at 11:53
  • 3
    I feel your pain. If it helps, Activator is in System not System.Reflection, and reflection permissions are not needed. – John Wu Apr 7 '17 at 12:25
  • 2
    This bypasses all compile time typechecking. The type argument is simply acting as a cast. Worse, all the parameters are now of one vacuous type and the factory accepts any number of them! While that is appropriate for certain circumstances, such as writing Dependency Injectors, it will likely make the code much harder to maintain in this case. Since maintenance is precisely this issue here (legacy code), this seems ill-advised. – Aluan Haddad Apr 8 '17 at 0:50
  • The original is just as type safe, but it needs to be more DRY. This is not type safe because the wrong arguments are easily passed and are not validated until runtime. If the wrong ones are passed, an exception will be thrown, not a compiler error. That could not happen with the original. – Aluan Haddad Apr 8 '17 at 0:53
0

As you said, the existing code has design problems as it is. First, having methods and constructors with so many (different!) parameters is a design smell.

If you don't fix the root cause of the complexity, you can make life easier by adding another layer of abstraction. This will ease maintaining the code base until you get time to refactor it (or move to a new one :) ).

  1. Make classes for the passed parameters (Param1, Param2, Param3).
  2. [Hack] Add a common interface (say IParameter) for the 3 classes.
    • It's a hack because the interface is not really common, but if you want to have only one switch in your Factory, that's a solution (the one I could find).
  3. Create a factory for the IParameter interface. The factory itself decides what to build based on the parameters it is set.
  4. [Hack] In the constructors of the existing classes, add a switch statement based on the concrete class type of the IParameter object.
    • It's a hack because this is not what interfaces should be used for. Interfaces should provide an abstraction for common behavior.

DonNet Fiddle with working example: https://dotnetfiddle.net/NoChOk

The code for perusal:

using System;

public class Program
{
    public void Main()
    {
        IHandler first = HandlerFactory.CreateInstance(
            HandlerType.Handler1,
            ParamBuilder.start().withOne("x").withTwo("y").build());
        IHandler second = HandlerFactory.CreateInstance(
            HandlerType.Handler2,
            ParamBuilder.start().withAlpha(1).withBeta(new DateTime()).build());
        IHandler third = HandlerFactory.CreateInstance(
            HandlerType.Handler3,
            ParamBuilder.start().withMickey(":)").withMouse(1L).build());

        first.DoStuff();
        second.DoStuff();
        third.DoStuff();
    }

    class HandlerFactory
    {
        public static IHandler CreateInstance(HandlerType type, IParam param)
        {
            switch (type) {
            case HandlerType.Handler1: return new Handler1(param);
            case HandlerType.Handler2: return new Handler2(param);
            case HandlerType.Handler3: return new Handler3(param);
            default: throw new NotSupportedException();
            }
        }
    }

    enum HandlerType { Handler1, Handler2, Handler3 
    }

    interface IHandler { void DoStuff(); }

    class Handler1 : IHandler {
        private Param1 numbers; private Param2 letters; private Param3 disney;

        public Handler1(IParam param) {
            if (param is Param1) numbers = (Param1) param;
            if (param is Param2) letters = (Param2) param;
            if (param is Param3) disney = (Param3) param;
        }

        public void DoStuff() { Console.WriteLine("I am alive - Handler 1");}
    }

    class Handler2 : IHandler {
        private Param1 numbers; private Param2 letters; private Param3 disney;

        public Handler2(IParam param) {
            if (param is Param1) numbers = (Param1) param;
            if (param is Param2) letters = (Param2) param;
            if (param is Param3) disney = (Param3) param;
        }

        public void DoStuff() { Console.WriteLine("I am alive - Handler 2");}
    }

    class Handler3 : IHandler {
        private Param1 numbers;
        private Param2 letters;
        private Param3 disney;

        public Handler3(IParam param) {
            if (param is Param1) numbers = (Param1) param;
            if (param is Param2) letters = (Param2) param;
            if (param is Param3) disney = (Param3) param;
        }

        public void DoStuff() { Console.WriteLine("I am alive - Handler 3");}
    }

    interface IParam {
        string One { get; } string Two { get; } // first param class

        int Alpha { get; } DateTime Beta { get; } // second param class

        string Mickey { get; } long Mouse { get; } // third param class
    }

    class ParamBuilder {
        private string one, two;
        private int? alpha; private DateTime? beta;
        private  string mickey; private long? mouse;

        public static ParamBuilder start() { return new ParamBuilder(); }

        public IParam build() {
            if (!string.IsNullOrEmpty(one) && !string.IsNullOrEmpty(one)) {
                return new Param1(one, two);
            }
            if (alpha.HasValue && beta.HasValue) {
                return new Param2(alpha.Value, beta.Value);
            }
            if (!string.IsNullOrEmpty(mickey) && mouse.HasValue) {
                return new Param3(mickey, mouse.Value);
            }

            throw new NotSupportedException();
        }

        public ParamBuilder withOne(string one) { this.one = one; return this; }
        public ParamBuilder withTwo(string two) { this.two = two; return this; }
        public ParamBuilder withAlpha(int alpha) { this.alpha = alpha; return this; }
        public ParamBuilder withBeta(DateTime beta) { this.beta = beta; return this; }
        public ParamBuilder withMickey(string m) { this.mickey = m; return this; }
        public ParamBuilder withMouse(long m) { this.mouse = m; return this; }
    }

    class Param1 : IParam {
        public string One { private set; get; }
        public string Two { private set; get; }

        public int Alpha { get {throw new NotImplementedException();} }
        public DateTime Beta { get {throw new NotImplementedException();} }
        public string Mickey { get {throw new NotImplementedException();} }
        public long Mouse { get {throw new NotImplementedException();} }

        public Param1(string one, string two) {
            One = one; Two = two;
        }
    }

    class Param2 : IParam {
        public int Alpha { private set; get; }
        public DateTime Beta { private set; get; }

        public string One { get {throw new NotImplementedException();} }
        public string Two { get {throw new NotImplementedException();} }
        public string Mickey { get {throw new NotImplementedException();} }
        public long Mouse { get {throw new NotImplementedException();} }

        public Param2(int alpha, DateTime beta) {
            Alpha = alpha; Beta = beta;
        }
    }

    class Param3 : IParam {
        public string Mickey { private set; get; }
        public long Mouse { private set; get; }

        public string One { get {throw new NotImplementedException();} }
        public string Two { get {throw new NotImplementedException();} }
        public int Alpha { get {throw new NotImplementedException();} }
        public DateTime Beta { get {throw new NotImplementedException();} }

        public Param3(string mi, long mo) {
            Mickey = mi; Mouse = mo;
        }
    }
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.