Puzzle 35: Minute by Minute

The following program simulates a simple clock. Its loop variable represents a millisecond counter that goes from 0 to the number of milliseconds in an hour. The body of the loop increments a minute counter at regular intervals. Finally, the program prints the minute counter. What does it print?

public class Clock {

    public static void main(String[] args) {

        int minutes = 0;

        for (int ms = 0; ms < 60*60*1000; ms++)

            if (ms % 60*1000 == 0)

                minutes++;

        System.out.println(minutes);

    }

}

Solution 35: Minute by Minute

The loop in this program is the standard idiomatic for loop. It steps the millisecond counter (ms) from 0 to the number of milliseconds in an hour, or 3,600,000, including the former but not the latter. The body of the loop appears to increment the minute counter (minutes) each time the millisecond counter is a multiple of 60,000, which is the number of milliseconds in a minute. This happens 3,600,000 / 60,000, or 60 times in the lifetime of the loop, so you might expect the program to print 60, which is, after all, the number of minutes in an hour. Running the program, however, tells a different story: It prints 60000. Why does it increment minutes so often?

The problem lies in the boolean expression (ms % 60*1000 == 0). You might think that this expression is equivalent to (ms % 60000 == 0), but it isn't. The remainder and multiplication operators have the same precedence [JLS 15.17], so the expression ms % 60*1000 is equivalent to (ms % 60) * 1000. This expression is equal to 0 if (ms % 60) is 0, so the loop increments minutes every 60 iterations. This accounts for the final result being off by a factor of a thousand.

The easiest way to fix the program is to insert a pair of parentheses into the boolean expression to force the correct order of evaluation:

if (ms % (60 * 1000) == 0)

    minutes++;

There is, however, a much better way to fix the program. Replace all magic numbers with appropriately named constants:

public class Clock {

    private static final int MS_PER_HOUR   = 60 * 60 * 1000;

    private static final int MS_PER_MINUTE = 60 * 1000;

    public static void main(String[] args) {

        int minutes = 0;

        for (int ms = 0; ms < MS_PER_HOUR; ms++)

            if (ms % MS_PER_MINUTE == 0)

                minutes++;

        System.out.println(minutes);

    }

}

The expression ms % 60*1000 in the original program was laid out to fool you into thinking that multiplication has higher precedence than remainder. The compiler, however, ignores this white space, so never use spacing to express grouping; use parentheses. Spacing can be deceptive, but parentheses never lie.