Re factoring a collection of IF statements that hold two arguments

Question

At the moment I have seven if statements that resemble the following code:

if(hit.collider.gameObject.tag == "Colour1" && start_time > look_at_time)
{
    new_colour1.ChangeObjectMaterialColour(hit.collider.gameObject.renderer.material.color);

   var colums = GameObject.FindGameObjectsWithTag("column");
   foreach( GameObject c in colums)
    c.GetComponent<MeshRenderer>().materials[1].color = new_colour1.orignalMaterial;
}


else if(hit.collider.gameObject.tag == "Colour2" && start_time > look_at_time)
{
    new_colour2.ChangeObjectMaterialColour(hit.collider.gameObject.renderer.material.color);
    var colums = GameObject.FindGameObjectsWithTag("column");
    foreach( GameObject c in colums)
       c.GetComponent<MeshRenderer>().materials[1].color = new_colour2.orignalMaterial;
}

Each statement is roughly 6 lines of code and takes up a lot of space and can be a little tricky to read. What I'm wanting to do is find a way to re factor this so that my code is little less clunky and doesn't take up too much space.

I had thought about changing my collection of if statements into a switch statement but I discovered that switch statements can't handle two arguments like I have above. If there any other way I can re factor my code but keep the same functionality or am I stuck with my collection of if statements?

edit

Updated to contain two of my 7 statements. Please note, I am trying to reduce the amount of IF statements I have, or find a more clever way to do them. I don't want to add any more extra lines of code or if statements.

Answer 1

I honestly think that the most readable and maintainable in your case is simply to move the logic in those if blocks into functions:

  if(hit.collider.gameObject.tag == "Colour1" && start_time > look_at_time)
    {
        DoTheThing(new_colour1);
    }
if(hit.collider.gameObject.tag == "Colour2" && start_time > look_at_time)
    {
        DoTheThing(new_colour2);
    }
//and so on



 void DoTheThing(MyType newColour)
  {
 newColour.ChangeObjectMaterialColour(hit.collider.gameObject.renderer.material.color);
      var colums = GameObject.FindGameObjectsWithTag("column");
       foreach( GameObject c in colums)
        c.GetComponent<MeshRenderer>().materials[1].color = newColour.orignalMaterial;
    }

As to using switch you can of course do something like this:

switch(hit.collider.gameObject.tag)
{
  case "Colour1":
      if (start_time>look_at_time)
      {
        //do something
      }
      else if (start_time<look_at_time)
      {
        //something else
      }
      else
      {
         //they are equal, etc, etc
      }
    break;
case "Colour2":
  //and so on
}

or if all of the if blocks make the same comparison between start_time and look_at_time then you could invert it:

enum TimeDifference
{
   LessThan,Equal,GreaterThan
}
//just one way to get a constant switchable value, not necessarily the best
var diff = TimeDifference.Equal;
if (start_time>look_at_time) {diff=TimeDifference.LessThan}
else if (start_time<look_at_time) {diff=TimeDifference.GreaterThan}

switch(diff)
{
  case TimeDifference.LessThan:
     switch(hit.collider.gameObject.tag)
     {
       case "Colour1":
         //do something for Colour1
        break;   
        case "Colour2":
         //do something for Colour2
        break;
      }//end switch Colour
     break;
  case TimeDifference.GreaterThan
    switch(hit.collider.gameObject.tag)
     {
       case "Colour1":
         //do something for Colour1
        break;   
        case "Colour2":
         //do something for Colour2
        break;
      }//end switch Colour
     break;  
   default:
    //nested switch for Colour in the == case, you get the idea.
}

although the readbility of the either of the above versus several nicely formatted and separate if blocks is very debatable.

Another approach is to use a Dictionary<string,Action<int,int>>: (assumingstart_time and look_at_time are both ints AND all of your logic involves them somehow)

var Actions = new Dictionary<string,Action<int,int>>();
Actions.Add("Colour1",(s,l)=>{
 if (start_time>look_at_time)
          {
            //do something
          }
          else if (start_time<look_at_time)
          {
            //something else
          }
          else
          {
             //they are equal, etc, etc
          }

});

and then your main code becomes 1 line:

Actions[hit.collider.gameObject.tag](start_time,look_at_time);

And I am sure that there are many many more ways you can rearrange that code.

Answer 2

Thanks for updating your code, now we can start looking at what is duplicated. For example, the only difference between the two blocks of code is the string "Color1" and "Color2" in the if statement, and the variable new_colour1 which is replaced with new_colour2.

From here I'd suggest something like the following:

//This should be declared once - e.g. class level not method level.
var colorDict = new Dictionary<string, [new_color_type]> 
                    {{"Colour1", new_colour1}, {"Colour2", new_colour2}};

[new_color_type] newColor;
if(start_time > look_at_time && colorDict.TryGetValue(
        hit.collider.gameObject.tag,
        out newColor))
{
   newColor.ChangeObjectMaterialColour(
                            hit.collider.gameObject.renderer.material.color);

   var colums = GameObject.FindGameObjectsWithTag("column");
   foreach( GameObject c in colums)
      c.GetComponent<MeshRenderer>().materials[1].color =
                                                    newColor.orignalMaterial;
}