Best design pattern for multiple if statements in an interface implementation

Question

I have an IComposer interface in my c# project:

public interface IComposer
{
    string GenerateSnippet(CodeTree tree);
}

CodeTree is a base class that contains a List<CodeTree> of classes that inherit from CodeTree. For example:

public class VariablesDecleration : CodeTree
{
     //Impl
}

public class LoopsDecleration : CodeTree
{
     //Impl
}

I can have a few classes that implement IComposer and in each I have the GenerateSnippet that loops over the List<CodeTree> and basically do:

foreach (CodeTree code in tree.Codes)
{
    if (code.GetType() == typeof(VariablesDecleration))
    {
        VariablesDecleration codeVariablesDecleration = (VariablesDecleration) code;
        // do class related stuff that has to do with VariablesDecleration
    }
    else if (code.GetType() == typeof(LoopsDecleration))
    {
        LoopsDecleration codeLoopsDecleration = (LoopsDecleration) code;
        // do class related stuff that has to do with LoopsDecleration
    }
}

I have this foreach and if statements repeating in each class that implements IComposer.

I was wondering if there is a better design pattern to handle such a case. lets say tommrow I add a new class that inherits from CodeTree - I would have to go over all the classes that implement IComposer and modify them.

I was thinking about the Visitor Design Pattern - but wasn't sure and not exactly sure if and how to implement it. Does Visitor even the right solution for this case?

Convocation

@DarshanPatel The syntax if not what matters to me in this question. I'm trying to understand the correct Design Pattern to make code readable and maintainable while keeping Open-Close principles. BTW - switch-case doesn't work with Type: A switch expression or case label must be a bool, char, string, integral, enum, or corresponding nullable type – Convocation

Visitor could very well be right. Can you provide more details (a name of the class, its relationship to the CodeTree base class, etc.) about the implementations of IComposer? It's not clear to me. – Splendor

There's a general refactoring that is the Strategy pattern, which is to replace if statements (on types) with a polymorphic call. refactoring.com/catalog/replaceConditionalWithPolymorphism.html However, in Narayana's answer you said "moving this logic to the data type is not an option" -- it's why we need more info on the concrete Composers (in your code). – Splendor

Answer 1

Move implementation related to the specific classes inside VariablesDecleration and LoopsDecleration, providing an abstract implementation in CodeTree. Then in your loops, simple call that method in CodeTree, without an if...else check.

public class VariablesDecleration : CodeTree
{
    //Impl
    public void SomeStuff()
    {
        //... specific to Variables
    }
}

public class LoopsDecleration : CodeTree
{
    //Impl
    public void SomeStuff()
    {
        //... specific to Loops
    }
}

public class CodeTree : ICodeTree
{
    void SomeStuff();
}

foreach (CodeTree code in tree.Codes)
{
    code.SomeStuff();
}

As per the comments, you might need something like this:

public interface IComposer
{
    string DoStuff();
}

public class LoopComposer1 : IComposer
{
    string DoStuff(){ .. }
}

public class VariableComposer1 : IComposer
{
    string DoStuff(){ .. }
}

public class ComposerCollection
{
    private IEnumerable<IComposer> composers;
    string GenerateSnippet()
    {
        foreach(var composer in composers)
        {
            composer.DoStuff();
        }
        ...
        ...
    }
}

Of course, now the relation has inverted, and your code tree or its creator has to define the composer collection for it.

Asphyxiate

Implementation depends on the type of the composer. The composer of type A would give different results than composer B for the same VariablesDecleration / LoopsDecleration object - so moving this logic to the data type is not an option. VariablesDecleration / LoopsDecleration should remain unaware of what's been done to their data. – Convocation

CodeTree needs to be abstract – Languishment

LoopsDecleration might produce result A when called from IComposer1 and produce B when called from IComposer2 - composers just need the info that LoopsDecleration hold to create the result. So the implementation can't be part of that object since it does not know who is using it. – Convocation

You should probably have List<Composers> instead of List<CodeTree>. You create Composer1, which has LoopComposer1, VariableComposer1 etc. each of which takes LoopDeclaration and VariableDeclaration in order to get created/initialized. Then you loop over LC1 and VC1, and call GenerateSnippet on that. As long as you have a list of something generic, but you want to do something specific, you will have to check for the type. – Asphyxiate

Answer 2

Your question is how you can perform actions on different types of code tree nodes, right?

Start by declaring a new interface called INodeActor which gives you the contract on how your code can act on code tree nodes. It's definition would look something like this:

public interface INodeActor
{
    bool CanAct(CodeTree treeNode);
    void Invoke(CodeTree treeNode);
}

Now you can take the following code:

foreach (CodeTree code in tree.Codes)
{
    if (code.GetType() == typeof(VariablesDecleration))
    {
        VariablesDecleration codeVariablesDecleration = (VariablesDecleration) code;
        // do class related stuff that has to do with VariablesDecleration
    }
    else if (code.GetType() == typeof(LoopsDecleration))
    {
        LoopsDecleration codeLoopsDecleration = (LoopsDecleration) code;
        // do class related stuff that has to do with LoopsDecleration
    }
}

And break it up:

public class VariablesDeclerationActor : INodeActor
{
    public void CanAct(CodeTree node)
    {
        return node.GetType() == typeof(VariablesDecleration);
    }

    public void Invoke(CodeTree node)
    {
         var decl = (VariablesDecleration)node;
         // do class related stuff that has to do with VariablesDecleration
    }
}

public class LoopsDeclerationActor : INodeActor
{
    public void CanAct(CodeTree node)
    {
        return node.GetType() == typeof(LoopsDecleration);
    }

    public void Invoke(CodeTree node)
    {
         var decl = (LoopsDecleration)node;
         // do class related stuff that has to do with LoopsDecleration
    }
}

Composer

Think of the composer to a work coordinator. It do not need to know how the actual work is done. It's responsibility is to traverse the code tree and delegate work to all registered actors.

public class Composer
{
    List<INodeActor> _actors = new List<INodeActor>();

    public void AddActor(INodeActor actor)
    {
        _actors.Add(actor);
    }

    public void Process(CodeTree tree)
    {
         foreach (CodeTree node in tree.Codes)
         {
             var actors = _actors.Where(x => x.CanAct(node));
             if (!actors.Any())
                 throw new InvalidOperationException("Got no actor for " + node.GetType());

             foreach (var actor in actors)
                 actor.Invoke(node);
         }
    }
}

Usage

You can customize the execution just as you like without having to change the traversal or any existing code. Thus the code is now following the SOLID principles.

var composer = new Composer();
composer.Add(new VariablesDeclerationActor());
composer.Add(new PrintVariablesToLog());
composer.Add(new AnalyzeLoops());

If you want to build a result you could introduce a context which is passed to the INodeActor invoke method:

public interface INodeActor
{
    bool CanAct(CodeTree treeNode);
    void Invoke(InvocationContext context);
}

Where the context contains the node to process, maybe a StringBuilder to store the result in etc. Compare it to HttpContext in ASP.NET.

Dewey

Answer 3

You can define a base class for all the composers and implement GenerateSnippet in it and avoid re-writing this code for each composer. In addition, you could improve your foreach loop by implementing composer.DoStuff(); as @Narayana suggested.

public class Composer:IComposer
{
    string GenerateSnippet()
    {
        foreach (CodeTree code in tree.Codes)
        {
             if (code.GetType() == typeof(VariablesDecleration))
             {
                 VariablesDecleration codeVariablesDecleration = (VariablesDecleration) code;
               // do class related stuff that has to do with VariablesDecleration
             }
             else if (code.GetType() == typeof(LoopsDecleration))
             {
                  LoopsDecleration codeLoopsDecleration = (LoopsDecleration) code;
                 // do class related stuff that has to do with LoopsDecleration
             }
        }
    }
}


public class ClassA: Composer
{

}

public class ClassB: Composer
{

}

Harrie

Answer 4

Perhaps Dependency Inversion could help?

class Program
{
    static void Main(string[] args)
    {
        var composer1 = new ComposerA(new Dictionary<Type, Func<CodeTree, string>>()
        {
            { typeof(VariablesDeclaration), SomeVariableAction },
            { typeof(LoopsDeclaration), SomeLoopAction }
        });

        var composer2 = new ComposerB(new Dictionary<Type, Func<CodeTree, string>>()
        {
            { typeof(VariablesDeclaration), SomeOtherAction }

        });

        var snippet1 = composer1.GenerateSnippet(new CodeTree() {Codes = new List<CodeTree>() {new LoopsDeclaration(), new VariablesDeclaration()}});
        var snippet2 = composer2.GenerateSnippet(new CodeTree() { Codes = new List<CodeTree>() { new VariablesDeclaration() } });

        Debug.WriteLine(snippet1); // "Some loop action Some variable action  some composer A spice"
        Debug.WriteLine(snippet2); // "Some other action  some composer B spice"
    }

    static string SomeVariableAction(CodeTree tree)
    {
        return "Some variable action ";
    }

    static string SomeLoopAction(CodeTree tree)
    {
        return "Some loop action ";
    }

    static string SomeOtherAction(CodeTree tree)
    {
        return "Some other action ";
    }
}

public interface IComposer
{
    string GenerateSnippet(CodeTree tree);
}

public class CodeTree
{
    public List<CodeTree> Codes;
}

public class ComposerBase
{
    protected Dictionary<Type, Func<CodeTree, string>> _actions;

    public ComposerBase(Dictionary<Type, Func<CodeTree, string>> actions)
    {
        _actions = actions;
    }

    public virtual string GenerateSnippet(CodeTree tree)
    {
        var result = "";

        foreach (var codeTree in tree.Codes)
        {
            result = string.Concat(result, _actions[codeTree.GetType()](tree));
        }

        return result;
    }
}

public class ComposerA : ComposerBase
{
    public ComposerA(Dictionary<Type, Func<CodeTree, string>> actions) : base(actions)
    {
    }

    public override string GenerateSnippet(CodeTree tree)
    {
        var result = base.GenerateSnippet(tree);

        return string.Concat(result, " some composer A spice");
    }
}

public class ComposerB : ComposerBase
{
    public ComposerB(Dictionary<Type, Func<CodeTree, string>> actions) : base(actions)
    {
    }

    public override string GenerateSnippet(CodeTree tree)
    {
        var result = base.GenerateSnippet(tree);

        return string.Concat(result, " some composer B spice");
    }
}

public class VariablesDeclaration : CodeTree
{
    //Impl
}

public class LoopsDeclaration : CodeTree
{
    //Impl
}

Icky

Answer 5

First of all, consider to move GenerateSnippet out of base class that other classes inherited from. Single responsibility principle of SOLID wants this. Composer must compose, and CodeTree must do just it's own work.

Next step is your if-else block. I think you can use simple Dictionary to store different types of your CodeTree items:

public interface IComposer {
    string GenerateSnippet(List<CodeTree> trees);
    void RegisterCodeTreeType<T>(T codeType) where T:CodeTree;
}

public abstract class ComposerBase {
    private readonly Dictionary<Type, CodeTree> _dictionary;

    public ComposerBase() {
        _dictionary = new Dictionary<Type, CodeTree>();
    }

    public void RegisterCodeTreeType<T>(T codeType) where T:CodeTree {
       _dicionary.Add(typeof(T), codeType);
    }

    public string GenerateSnippet(List<CodeTree> trees) {
        StringBuilder fullCode = new StringBuilder();
        foreach(var tree in trees) {
            fullCode.Append(_dictionary[tree.GetType()].GenerateSnippet();
        }
    }
}

Hope you catch an idea. You should register all types with Composer RegisterCodeTreeType method on application start. Now it doesn't depend on how many types you have. Please note, it's just a quick code, use it carefully.

Turro

Answer 6

Ok, having to write those If statements on type- checking is bad as you realized, and it defeats the purpose of abstraction and sub-classing as you already did.

You want your calling code to remain the same. (OCP).

Right now the CodeTree object is taking the responsibility of figuring out the logic of each of the concrete implementations. The issue is that the responsibility belongs on each concrete. Your for loop should just be casting to the interface type IComposer and calling the method string GenerateSnippet(CodeTree tree); to get the result. Each concrete should handle the implementation detail. Instead of looping through CodeTree objects you should be looping through IComposer objects.

Moving the implementation to a specific type object would not require you to make changes in the execution code, but rather just extend by adding a new type if it ever happens. If your implementation detail is vastly different, you could look into the Type Object Pattern. that will handle all the detail and your CodeTree object could remain simpler.

Winterkill