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 containing a List<CodeTree> classes that inherit from CodeTree . For example:

 public class VariablesDecleration : CodeTree { //Impl } public class LoopsDecleration : CodeTree { //Impl } 

I can have several classes that implement IComposer , and in each of them I have a GenerateSnippet that traverses the List<CodeTree> and basically does:

 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 the following foreach and if repeated in every class that implements IComposer .

I was wondering if there is a better design to handle such a case. let's say tommrow I am adding a new class that inherits from CodeTree - I will need to go through all the classes that implement IComposer and modify them.

I was thinking about the visitor design template, but I was not sure and not quite sure what and how to implement it. Does the visitor even present the right solution for this case?

Answer 1

Move the implementation associated with specific classes inside VariablesDecleration and LoopsDecleration , providing an abstract implementation in CodeTree . Then in your loops just call this method in CodeTree without checking if ... else.

 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(); } 

According to 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 is inverted, and your code tree or its creator must define the composer's collection for it.

Answer 2

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

Start by announcing a new interface called INodeActor , which gives you a contract on how your code can affect nodes in the code tree. This definition will 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 get upset:

 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 a composer for a work coordinator. He does not need to know how the actual work is done. The responsibility is to cross the code tree and delegate the work to all registered participants.

 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); } } } 

Using

You can customize the execution as you like without changing the workaround or any existing code. Thus, the code now follows the principles of SOLID.

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

If you want to build the result, you can enter the context that is passed to the INodeActor invoke method:

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

If the context contains a node for processing, perhaps StringBuilder to store the result, etc. Compare it to the HttpContext in ASP.NET.

Answer 3

You can define a base class for all composers and implement GenerateSnippet in it and not rewrite this code for each composer. Alternatively, you can improve your foreach loop by executing the composer .DoStuff (); as suggested by N. Narayana.

 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 { } 

Answer 4

Maybe dependency inversion can 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 } 

Answer 5

First of all, consider porting GenerateSnippet from a base class that has inherited other classes. The Lone Solid Responsibility Principle wants this. The composer should compose, and CodeTree should do only his work.

The next step is your if-else block. I think you can use a simple dictionary to store various types of CodeTree elements:

 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(); } } } 

I hope you get the idea. You must register all types with the Composer RegisterCodeTreeType method when the application starts. Now it does not depend on how many types you have. Please note that this is just a quick code, use it carefully.

Answer 6

Well, you need to write those If statements that check the type, badly, as you understand, and it defeats the goal of abstraction and subclassification, as you already did.

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

Currently, the CodeTree object takes responsibility for determining the logic of each particular implementation. The problem is that the responsibility lies with each individual. The for loop should simply be discarded on the IComposer interface IComposer and call the string GenerateSnippet(CodeTree tree); method string GenerateSnippet(CodeTree tree); to get the result. Each specific must handle implementation details. Instead of iterating over CodeTree objects, you should iterate over IComposer objects.

Moving an implementation into an object of a certain type does not require changes to the execution code, but simply expands by adding a new type if it ever happens. If the implementation details vary significantly, you can see the Object Type . which will handle all the details, and your CodeTree object can stay simpler.