patterncsharpModerate
Nested if statements with 3 different parameters
Viewed 0 times
withstatementsnesteddifferentparameters
Problem
I have a block of code below. The
-
-
-
After those options, if all enabled (
If one single enabled method returns
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
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.
Look at this. You are inspecting
Simplify that to
And then simplify that to leaving it out altogether.
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!
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.