none
Best way to structure an if statement in the following: RRS feed

  • Question

  • Hi, I'm questioning the way I am implementing this if statement:

    if ((!queuedForDeletion && !diagnosed) && !healthy)
    {
       // Code
    }
    
    else if (diagnosed)
    {
       if (healthy)
       {
          // Code
       }
    
       else
       {
          // Code
       }
    }
    
    else if (queuedForDeletion)
    {
       // Code
    }
    
    else
    {
       Throw.Exception
    }

    I am questioning the flow of this if statement. Is this the way you would structure this if? If not, how would you expect it to be structured?



    Wednesday, August 7, 2019 7:48 PM

Answers

  • Hi, I will attempt to summarize my actual programs interactions:

    Here is a (stripped down) Employee class:

    public class Employee()
    {
       private bool isResting = false;
    
       public void UpdateTask(Workplace workplace, bool workplaceUnavailable = false, bool taskReassigned = false)
       {
          if ((!workplaceUnavailable && !taskReassigned) && !isResting
          {
             GoToTask(workplace.position); // Goes to the new location
          }
    
          else if (workplaceUnavailable || taskReassigned)
          {
             UnsubscribeFromWorkplace(); // Unsubscribes from the delegate
    
             if (!isResting)
             {
                GoToStaffroom(); // Go to the Staffroom position
             }
          }
    }          

    (I realized that the if statement this thread was created for could be written as above)

    The UpdateTask method is subscribed to a delegate in the Workplace object. I use the delegate since I don't want a the Workplace to know which Employee is responsible for it, just that it either has an Employee assigned or doesn't).

    Then, based on different states in the Workplace object (such as being moved, reassigned or closed) I call the delegate and provide the arguments. For example, if the Workplace is being destroyed:

    public class Workplace
    {

    private void OnDestroy() { SetWorkplaceState(this, workplaceUnavailable : true); // The delegate }
    }


    I understand that passing state as booleans can be seen as a code smell, and that I could separate UpdateTask into separate methods, and is something I am looking at refactoring.

    Could you elaborate on what you mean by using variables instead of checking object state? In my mind, variables are what define states? Do you mean explicitly using the object like the following:

    if (employee.IsResting)

    as opposed to:

    if (isResting)
    Thanks for your time.

    Thursday, August 8, 2019 9:20 AM

All replies

  • Your first if looks suspect to me. It executes if all the flags are false. If any of them aren't then it steps into the remainder of the ifs which handle diagnosed and queued. But let's walk through the 8 combinations that can happen here

    queuedForDeletion diagnosed healthy if
    F F F 1
    F F T exception
    F T F 3
    F T T 2
    T F F 4
    T F T 4
    T T F 3
    T T T 2

    It seems like `healthy` doesn't matter if `diagnosed` is `false`. Beyond that state all combinations are handled but there are some dups. It seems like you could change your final else to be same as the first `if` and then either special case `healthy` (which is not `diagnosed`) or ignore it if it isn't a possible situation.


    Michael Taylor http://www.michaeltaylorp3.net

    Wednesday, August 7, 2019 9:01 PM
    Moderator
  • Hi, thanks for your reply.

    I like your explanation. Although it didn't appear that the situation where !diagnosed && healthy matters, because I never handled it, it does. But based on your answer I came up with this:

    if (diagnosed)
    {
       if (healthy)
       {
          // Code
       }
    
       else
       {
          // Code
       }
    }
    
    else if (queuedForDeletion)
    {
       // Code
    }
    
    else
    {
       if (!healthy)
       {
          // Code
       }
    }
    

    My only problem with this is I've been told that the most common code should not go in the else statement, and based on my program the else would be the most common situation. Maybe that's why I tried to force the first 'if'.

    Wednesday, August 7, 2019 9:36 PM
  • Yes, in general your more likely scenarios should occur first. However if you have several conditions that are mutually exclusive then it sometimes becomes necessary to adjust the ordering. Your last else/if can be simply an `else if`. Note that your new code doesn't handling all the cases anymore and therefore isn't equivalent to your original post. Not sure if that matters to you. You've also changed the semantics a little. If `diagnosed` is `true` then `queuedForDeletion` no longer matters. You might consider running back through the combination table to make sure all the cases behave the way you want.

    Michael Taylor http://www.michaeltaylorp3.net

    Wednesday, August 7, 2019 9:51 PM
    Moderator
  • Greetings Jay.

    My 2c worth is that you shouldn't overthink these situations. The important thing is that the code flows in a way that seems natural and easy to read. What that means in practice is very subjective, but if you find yourself chopping your code to make it conform to arbitrary rules like what gets executed most often, then you're probably stuffing it up.

    Wednesday, August 7, 2019 11:32 PM
  • This code is not problem but you need to know what each variable means. There is not context of problem so we can create many solutions but each solution will make sence for each person. You need to find data representation and description which will describe solution clearly. 

    By your code it seems like procedural programing instead of object oriented. It is because you use variables instaead of checking object state. If you have else branch with throw exception it doesn't make sence because object state is invalid. Who can check if object state is valid? Object itself when state is changed or when object is created and state is setted up. 

    Problem of this code is that you check object state in one place. As project grows there will be another places when same object state will be checked and same logic will be on many places. 

    Thursday, August 8, 2019 3:46 AM
  • Hi, I will attempt to summarize my actual programs interactions:

    Here is a (stripped down) Employee class:

    public class Employee()
    {
       private bool isResting = false;
    
       public void UpdateTask(Workplace workplace, bool workplaceUnavailable = false, bool taskReassigned = false)
       {
          if ((!workplaceUnavailable && !taskReassigned) && !isResting
          {
             GoToTask(workplace.position); // Goes to the new location
          }
    
          else if (workplaceUnavailable || taskReassigned)
          {
             UnsubscribeFromWorkplace(); // Unsubscribes from the delegate
    
             if (!isResting)
             {
                GoToStaffroom(); // Go to the Staffroom position
             }
          }
    }          

    (I realized that the if statement this thread was created for could be written as above)

    The UpdateTask method is subscribed to a delegate in the Workplace object. I use the delegate since I don't want a the Workplace to know which Employee is responsible for it, just that it either has an Employee assigned or doesn't).

    Then, based on different states in the Workplace object (such as being moved, reassigned or closed) I call the delegate and provide the arguments. For example, if the Workplace is being destroyed:

    public class Workplace
    {

    private void OnDestroy() { SetWorkplaceState(this, workplaceUnavailable : true); // The delegate }
    }


    I understand that passing state as booleans can be seen as a code smell, and that I could separate UpdateTask into separate methods, and is something I am looking at refactoring.

    Could you elaborate on what you mean by using variables instead of checking object state? In my mind, variables are what define states? Do you mean explicitly using the object like the following:

    if (employee.IsResting)

    as opposed to:

    if (isResting)
    Thanks for your time.

    Thursday, August 8, 2019 9:20 AM
  • Greetings Jay.

    My 2c worth is that you shouldn't overthink these situations. The important thing is that the code flows in a way that seems natural and easy to read. What that means in practice is very subjective, but if you find yourself chopping your code to make it conform to arbitrary rules like what gets executed most often, then you're probably stuffing it up.

    Thanks, I do find myself getting caught up in trying to adhere to practices. My code ultimately does work as intended, so I'm going to try to reel it in.
    Thursday, August 8, 2019 9:24 AM
  • Yes, in general your more likely scenarios should occur first. However if you have several conditions that are mutually exclusive then it sometimes becomes necessary to adjust the ordering. Your last else/if can be simply an `else if`. Note that your new code doesn't handling all the cases anymore and therefore isn't equivalent to your original post. Not sure if that matters to you. You've also changed the semantics a little. If `diagnosed` is `true` then `queuedForDeletion` no longer matters. You might consider running back through the combination table to make sure all the cases behave the way you want.

    Michael Taylor http://www.michaeltaylorp3.net

    Thank you. You really have been a huge help.
    Thursday, August 8, 2019 9:27 AM
  • Hi

    Is your problem solved? If so, please post "Mark as answer" to the appropriate answer. So that it will help other members to find the solution quickly if they face the similar issue.

    Best Regards,

    Jack


    MSDN Community Support
    Please remember to click "Mark as Answer" the responses that resolved your issue, and to click "Unmark as Answer" if not. This can be beneficial to other community members reading this thread. If you have any compliments or complaints to MSDN Support, feel free to contact MSDNFSF@microsoft.com.

    Wednesday, August 21, 2019 8:08 AM
    Moderator