11
\$\begingroup\$

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().

\$\endgroup\$
  • \$\begingroup\$ Can you not change the 4 pairs of variables (runPartN and cmdPartN) into an array of an appropriate structure type? Then you could loop over the array. \$\endgroup\$ – Jonathan Leffler Feb 4 '11 at 6:38
  • \$\begingroup\$ After looking at the responses, yea I realize I could do that now. But that may be a little more complicated than what I need (read my answer updated edit in my original post). But as I stated if I need a more detailed answer in the future, that is the route I may want to go. \$\endgroup\$ – YourRedHerring Feb 4 '11 at 8:31
11
\$\begingroup\$

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();
\$\endgroup\$
  • \$\begingroup\$ AH, this one is good too as it checks for those not run. Something that may be useful as well. \$\endgroup\$ – YourRedHerring Feb 3 '11 at 21:35
  • 3
    \$\begingroup\$ Then refactor runCompleted to: return !checked || done == "done";. Anytime you have if (x) return true else return false;, you can factor it to return x; (and in this case x can be simplified a bit as well). \$\endgroup\$ – Jerry Coffin Feb 3 '11 at 22:50
  • \$\begingroup\$ @Jerry: Good point, I was throwing it together quick when I was leaving work. I'll update with a new version of runCompleted and leave the old as well. \$\endgroup\$ – Mark Loeser Feb 4 '11 at 1:33
  • \$\begingroup\$ I think this is the closest to what I need to accomplish. This code also makes it rather easier to add newer parts if needed compared to the other answers. Although if I need to make it any more complex I may end up using @Michael's answer using the interface. Marking as the answer unless anyone else has any other input \$\endgroup\$ – YourRedHerring Feb 4 '11 at 8:22
  • \$\begingroup\$ The string comparison should be written done.Equals("done", StringComparison.Ordinal). You could choose a different StringComparison operator, but in general when doing a string == comparison it is better practice to call the .Equals method on the string object with a specifiedStringComparison (see MSDN "Using an overload explicitly stating the StringComparison type is still recommended, even if you desire an ordinal comparison") This is something that FxCop specifically looks for. \$\endgroup\$ – pstrjds Feb 9 '11 at 20:35
16
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ Wow, how did I not see that? I think what I meant was for it to check the current value so if it was ever set to false it stayed false. I see now exactly what you mean though. Thanks for pointing it out \$\endgroup\$ – YourRedHerring Feb 3 '11 at 22:28
  • \$\begingroup\$ And the double 'if' can surely be abbreviated to if (runPart1.Checked && cmdPart1 != "done")? \$\endgroup\$ – Jonathan Leffler Feb 4 '11 at 6:39
  • \$\begingroup\$ Yea the double if I know I could have combined. I had it separated when I was still trying to work it out originally and never combined the two. Probably should have before asking this. \$\endgroup\$ – YourRedHerring Feb 4 '11 at 8:09
3
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ Ah, you can do "*"? I guess I'm showing my lack of C# foo, since I've never touched the language. \$\endgroup\$ – Mark Loeser Feb 3 '11 at 21:12
  • \$\begingroup\$ @Mark: No, that's just a placeholder. I'll edit that. :) \$\endgroup\$ – Michael K Feb 3 '11 at 21:14
  • \$\begingroup\$ Wow this is clever. I do like this. But if part 3 return false and 4 is true, then in the end it will be true. It needs to be that if at any time it returns false, it stays false. So if Part 2 is false, the end result will be false even if 3 and 4 are true. if any are false then it stays false. \$\endgroup\$ – YourRedHerring Feb 3 '11 at 21:22
  • \$\begingroup\$ It will be - false && true == false. So if part 3 sets allCompleted false, part 4 will not change that. \$\endgroup\$ – Michael K Feb 3 '11 at 21:24
  • \$\begingroup\$ OOh yea thats true, good point. Thats great. I like this example. Did the other person delete his answer? \$\endgroup\$ – YourRedHerring Feb 3 '11 at 21:27
2
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ what if the last statement is like this? ` for (int i = 0; i < checkBoxes.Length; i++)` ` allCompleted &= (checkBoxes[i].Checked && commands[i] != "done")` ` ` ` if (allCompleted)` ` allDone();` ok sorry I cant seem to get code working in the comments. but the above is mixing in a little of what was done in @Michael's answer \$\endgroup\$ – YourRedHerring Feb 3 '11 at 22:08
  • \$\begingroup\$ @user1402: If the checkbox is not checked, then the expression will evaluate to false, and allCompleted &= false will set allCompleted to false prematurely. \$\endgroup\$ – dreamlax Feb 3 '11 at 22:12
  • \$\begingroup\$ Basically, you need to inspect both the checkbox and the string before even considering changing the allCompleted variable. If the checkbox is not checked, then the second part of the && expression will not be evaluated. \$\endgroup\$ – dreamlax Feb 3 '11 at 22:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.