Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Just saw this at work in a code review and was wondering if this style of coding - for loop, doing && like that, seems fine to you or is there a better way of doing the same?

for (int j = 1; j <= ETtop; j++)
{ 
    // Check everything.
    int k = ET[j].PriIdx;
    for (int z = 1; z <= ET[k].ArgTop; z++ )
    {
        // Check to make sure every Argument in the equation is valid.
        // If it's a calc this is only true if it's been evaluated.
        ready = ready && ET[k].Arg[z].Valid;
    }
}
share|improve this question
1  
Yep seems ok to me. Perhaps minor changes might be changing j, z and k to an actual meaning if relevant. –  dreza Nov 8 '12 at 22:20
1  
Only thing you could do is change to ready &= ET[k].Arg[z].Valid, but that might lower readability for some. I'd improve the variable names though, but that's another topic. :) –  Lars-Erik Nov 9 '12 at 9:57

2 Answers 2

Currently, once you get ready to be false, all other checks after that will fail (as false && condition2 will always be false).

Why not just break out of the inner for loop when ET[k].Arg[z].Valid is false? If ready is needed, you may keep the assignment line, but break when it's false.

share|improve this answer
    
somewhere above the code that I posted there are some areas that ready can be set to True...so in this case does your answer still correct? –  yekAdami Nov 8 '12 at 21:50
    
Nope. If ready can be changed back to true, then the && condition is probably necessary. The optimization may be that you could break out earlier depending on when you check things . . . –  ernie Nov 8 '12 at 21:53
    
@ernie : As this is code review, it would be appreciated if you posted your rewritten version of the code. –  user19024 Nov 8 '12 at 23:52
    
@yekAdami I should clarify, if ready can be changed in the inner for-loop, then the code is likely fine. –  ernie Nov 9 '12 at 0:29

If possible, I would rewrite your code to use collections instead of indexers and top properties. That way, you could use foreach instead of for, or, even better, LINQ:

ready = ET.All(e => e.Arg.All(arg => arg.Valid))

or:

ready = (from e in ER
         from arg in e.Arg
         select arg.Valid)
       .All();

This most likely won't make your code faster (although it does use short-circuit evaluation), but it will make it more readable, IMHO.

share|improve this answer
    
Combining this answer with ernie's, perhaps it could instead be ready = !ET.Any(e => e.Arg.Any(arg => !arg.Valid)). –  Dan Lyons Nov 9 '12 at 18:54
    
@DanLyons I think that only makes things more complicated for no benefit. –  svick Nov 10 '12 at 0:27

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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