views:

125

answers:

5

What am I doing wrong here?

I am wanting to display integers from 1-100 who are divisible by either 6 or 7. That's done and working. The next step is to not display any that are divisible by both...that isn't working in my loop (those integers are still displaying)

for (int i = 1; i < 100; i++)
    if (i % 6 == 0 || i % 7 == 0 && i % (6 * 7) != 0){
        println(i);
    }

Thanks! Joel

+5  A: 
hatchetman82
in general, dont ever assume anything about order of evaluation or "stickiness" of logical operators, always add (...)
hatchetman82
great. Thanks guys!
Joel
+2  A: 

Missing parentheses:

for (int i = 1; i < 100; i++) {
    if ((i % 6 == 0 || i % 7 == 0) && i % (6 * 7) != 0) {
        println(i);
    }
}
Mark Byers
Thanks-that did it!
Joel
+1  A: 
for (int i = 1; i < 100; i++)
if ((i % 6 == 0 || i % 7 == 0) && !(i % 6 == 0 && i % 7 == 0)){
    println(i);
}
objects
This does NOT compile! Wrong position of the closing parenthesis here `(i % 6 == 0 || i % 7) == 0` should be `(i % 6 == 0 || i % 7 == 0)`
Carlos Heuberger
oops, thanks for that
objects
+2  A: 

I would simply stop worrying about how to evaluate precedence, and use something like:

for (int i = 1; i <= 100; i++) {
    if ((i % 42) == 0) continue;
    if ((i %  6) == 0) println (i);
    if ((i %  7) == 0) println (i);
}

I'm assuming here that 1-100 was an inclusive range in which case you should use <= rather than <. It won't matter for your specific case since 100 is divisible by neither 6 nor 7.

Guess what? Any decent optimising compiler (including JIT ones) will probably end up generating the same code as for all the other possibilities. And, even if it didn't, it wouldn't matter unless you were calling this function a great many times.

I think that's a tiny bit more readable than:

if (i % 6 == 0 || i % 7 == 0 && i % (6 * 7) != 0) ...

or, worse yet, the Lisp-like thing you'll have to turn it into to get it working properly :-)


Keep in mind one possibility - you can change your loop to make it more efficient (sevenfold), for the specific case with 6 and 7, thus:

for (int i = 7; i <= 100; i += 7)
    if ((i % 6) != 0)
        println (i);

This uses the for loop itself to only check multiples of 7 and print them if they're not also multiples of 6.

paxdiablo
+1  A: 

You could also use the exclusive or

    for (int i = 1; i < 100; i++) {
        if ((i % 6 == 0) ^ (i % 7 == 0)) {
            println(i);
        }
    }

or just an unequal if ((i % 6 == 0) != (i % 7 == 0))
Since exclusive or is not used that often, I doubt this will increase the readability of the code...

Carlos Heuberger