0

I'm having a hard time trying to implement a SOLID principle regarding conditions inside loops. For example I have a List of objects inside an object.

public class ClassTwo{
      int id;
      string name;
      string quantity;
      bool markasDelete;
    } 
public class SampleClass{
      string name;
      List<ClassTwo> listClassTwo;
      ///other initializations, props, method ,construct

      void Process(List<ClassTwo> newListOfClassTwo){
         foreach(ClassTwo c in newListOfClassTwo){
               if(c.id == 0) 
               {
                  ///do other things
                  listClassTwo.Add(c);
               }
               else if(c.id > 0 && !markasDelete)
               {
                  ///do update things
               }
               else
               {
                  ///do delete things
               }
         }

      }
    }

As you can see from above my "Process" method is doing too much. I tried to reduce the work segragating each condition to a specific method but im very unhappy with it. My code now looks something like this.

void Process(List<ClassTwo> newListOfClassTwo){
   foreach(ClassTwo c in newListOfClassTwo){
       AddMethod(c);
       UpdateMethod(c);
       RemoveMethod(c);

   }
}

Can please someone help me on this?

zxc
  • 1,504
  • 3
  • 13
  • 32
  • I would use the first code, but with replaced content to functions `AddMethod`. `UpdateMethod`, `RemoveMethod`. I'm not sure if it is correct as SOLID but, there should be no problem with this. – Julo Sep 25 '18 at 05:46
  • Why are you performing add, update and delete at the same time inside your process? Which part of the SOLID principle are you trying to address? – jegtugado Sep 25 '18 at 05:46
  • @Julo do you mean the loops should be inside the 3 methods? – zxc Sep 25 '18 at 05:54
  • @JohnEphraimTugado im trying to address the single responsibility.. – zxc Sep 25 '18 at 05:55
  • 1
    I mean: `foreach(ClassTwo c in newListOfClassTwo){ if(c.id == 0) { AddMethod(c); } else if(c.id > 0 && !markasDelete) { UpdateMethod(c); } else { RemoveMethod(c); } } ` – Julo Sep 25 '18 at 05:59
  • Take a look at [Single Responsibility Principle](https://stackoverflow.com/a/7591887/6138713) – jegtugado Sep 25 '18 at 06:20
  • One approach is to inject a handler for each of the actions (add/update/remove) leaving the `Process` method with the single responsibility of acting as [Mediator](https://www.dofactory.com/net/mediator-design-pattern) – qujck Sep 25 '18 at 09:13
  • Do you modify `c.id` before adding it to `listClassTwo` ? – Spotted Sep 25 '18 at 09:16
  • I'm not sure what you're trying to achieve finally but definitely your code in `Process` method is driven by the state `List listClassTwo`. So in order to get better responsibilities assignments you need to start with this member - decompose it if you don't like doing all things in one or consider getting rid of this state at all. – Dmytro Mukalov Sep 25 '18 at 10:39
  • 1
    There's no problem with your code. Let's not over-engineer simple code. Apply SRP at class level. It is applicable at method level too, but not required in your case. [KISS](https://en.wikipedia.org/wiki/KISS_principle)! – Nikhil Vartak Sep 25 '18 at 17:56

0 Answers0