HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Nested if statements with 3 different parameters

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withstatementsnesteddifferentparameters

Problem

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 ru

Solution

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!

Code Snippets

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

Context

StackExchange Code Review Q#590, answer score: 16

Revisions (0)

No revisions yet.