Nested if statements with 3 different parameters

Question

I have a block of code below. The allDone() method at the bottom should only be run if the allCompleted == true. It should run through each of the statements to test.

• allCompleted: This starts as true so the below logic works right.

• run*.Checked: This is based on a check box in a form. This block should only run if this box is checked.

• cmd: This is a generic string variable stating whether another part of the code (not shown here) was run successfully. If it has run successfully this string will read "done".

After those options, if all enabled (run*.Checked == true) methods have returned the cmd* string as "done" (everything that's checked has run successfully) then allCompleted should be true at the end so allDone() gets run.

If one single enabled method returns false (there was an error somewhere or otherwise it did not return "done"), then the allDone() method should not be run and the code will continue, skipping the last if (allCompleted) statement.

bool allCompleted = true;

if (runPart1.Checked)
    if (cmdPart1 == "done")
        allCompleted = ((allCompleted)? true : false);
    else
        allCompleted = false;

if (runPart2.Checked)
    if (cmdPart2 == "done")
        allCompleted = ((allCompleted) ? true : false);
    else
        allCompleted = false;

if (runPart3.Checked)
    if (cmdPart3 == "done")
        allCompleted = ((allCompleted) ? true : false);
    else
        allCompleted = false;

if (runPart4.Checked)
    if (cmdPart4 == "done")
        allCompleted = ((allCompleted) ? true : false);
    else
        allCompleted = false;


if (allCompleted)
    allDone();

So if at anytime one of the enabled parts fail the code will basically just move on.

As it stands this code works, I just feel like it could be written better. Is this the best way or have I got it? Something about it makes me feel awkward still.

EDIT: Also, each time one of the parts completes, it runs this method, so it will run a few times being false in the end until the last one runs and all the others are "done" in which case it should completes and run allDone().

Answer 1

Okay, here is how I would reduce the code duplication (if I am understanding the conditions correctly):

Edit: Original:

bool runCompleted(bool checked, string done)
{
    if( ( checked && done == "done" ) || !checked )
        return true;
    else
        return false;
}

New version based on Jerry's feedback:

bool runCompleted(bool checked, string done)
{
    return !checked || done == "done";
}

Then in your code:

if(    runCompleted(runPart1.Checked, cmdPart1 )
    && runCompleted(runpart2.Checked, cmdPart2 )
    && runCompleted(runpart3.Checked, cmdPart3 )
    && runCompleted(runpart4.Checked, cmdPart4 )
  )
    allDone();

Answer 2

Others are giving you refactoring ideas, so I will just focus on one statement in your original code that is repeated 4 times.

allCompleted = ((allCompleted) ? true : false); 

Look at this. You are inspecting allCompleted. If the value is true, you're setting it to true. If it is not true, you're setting it to false. You are setting it to what it already is in a sort of non-intuitive way. You could very well rewrite it as the below and have the exact same meaning.

allCompleted = allCompleted ? allCompleted : allCompleted;

Simplify that to

allCompleted = allCompleted;

And then simplify that to leaving it out altogether.

if(runPart1.Checked)
   if (cmdPart1 != "done")
      allCompleted = false;

Code can be complicated enough as it is. Try not to add further complexity by including code that can be non-obvious in the fact that it does nothing at all!

Answer 3

allCompleted = true;
allCompleted &= (!runPart1.Checked || cmdPart1 == "done"));
allCompleted &= (!runPart2.Checked || cmdPart2 == "done"));
allCompleted &= (!runPart3.Checked || cmdPart3 == "done"));
allCompleted &= (!runPart4.Checked || cmdPart4 == "done"));

if (allCompleted) {
   allDone();
}

Here's a start - without more context I'm not sure what else can be done. Will edit other ideas later.

EDIT: Possible idea for you to try:

interface RunPart {
   public boolean doAction();
}

And for the action code:

ArrayList CheckActions = new ArrayList();

if (RunPart1.Checked)
   CheckActions.add(new RunPart1());

if (RunPart2.Checked)
   CheckActions.add(new RunPart2());

if (RunPart3.Checked)
   CheckActions.add(new RunPart3());

if (RunPart4.Checked)
   CheckActions.add(new RunPart4());

foreach (RunPart runPart in CheckActions) {
   allCompleted &= part.doAction();
}

if (allCompleted) {
   allDone();
}

All the RunParts need to implement the RunPart interface. This will make it a bit easier to add more actions in the future. Not sure if this is practical for you or not but here it is.

Answer 4

Pull that block of code out into its own method if it isn't already, and then just do:

if (runPart1.Checked && (cmdPart1 != "done")) return;
if (runPart2.Checked && (cmdPart2 != "done")) return;
if (runPart3.Checked && (cmdPart3 != "done")) return;
if (runPart4.Checked && (cmdPart4 != "done")) return;

allDone();

This isn't C. We don't have to be afraid of early returns anymore.

Answer 5

I'm a very big noob at C# so please forgive me if this is a horrible solution, but how about using arrays?

bool allCompleted = true;
String[] commands = { cmdPart1, cmdPart2, cmdPart3, cmdPart4 };
CheckBox[] checkBoxes = { runPart1, runPart2, runPart3, runPart4 };

// ensure commands.Length == checkBoxes.Length
for (int i = 0; i < checkBoxes.Length; i++)
    if (checkBoxes[i].Checked && commands[i] != "done")
        allCompleted = false;