# Is this a right and effcient way of checking AND in a for loop

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.
}
}

-
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
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

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.

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

-
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