I've heard that some "break"s aren't bad practice. What about this one?

Question

Hey everyone,

I have often heard that using breaks in Java is considered bad practice, but after reading some threads on Stack Overflow, I've seen otherwise. Many say that it is acceptable in certain cases.

I'm a little confused as to what is/isn't bad practice in this case.

For Project Euler: Problem 7, I've constructed the code below. The challenge was to find the 10001st prime.

int index = 2, count = 1, numPrime = 1;

while (true) {
    index++;

    if (isPrime(index)) {
        count = index;
        numPrime++;
    }

    if (numPrime >= 10001)
        break;
}

System.out.println(count);

This returns the correct answer (in 21ms), but am I overlooking a serious warning? It's 100% possible to create a while loop without a break, but I find that this is a little easier to follow.

Is the way I use the break; bad practice? I know that there's always a way around using one, but is it really that terrible here?

Many thanks

Justian

EDIT

Here's my isPrime() code. I might as well optimize this while I'm at it.

public static boolean isPrime(long num) {  
    if (num == 2)
        return true;

    if (num % 2 == 0 || num <= 0)
        return false;

    for (long i = 3; i * i <= num; i += 2)
        if (num % i == 0)
            return false;

    return true;
}

Answer 1

I'm not sure that breaks in general are bad practice, but I think that this one is.

It's a bit silly because it's unnecessary. Your while (true) ... break is entirely equivalent to:

while (numPrime < 10001) {
   ...
}

but much less intuitive.

Sticking with the canonical way to represent things means less computational overhead for your readers, makes your code easier to understand, thus easier to maintain and ultimately more robust.

Edit (in response to comment): You're right that there's always a way around using one, but the rule (if you want one) is relatively simple: write whatever is most readable. In the case you posted, this is not the most readable as the alternative I gave is the canonical way to represent this. In some cases, for example looping though a collection until you find a specific candidate that matches, using break probably is the most fitting way to represent this pattern.

It would be difficult (and I woudl argue, futile) to try to come up with hard-and-fast rules about when to use break and when to pull out variables and use conditionals instead. Often it's easy to tell what's the simplest option, but not always. This is one of those examples where experience really counts - the more good/bad code you've read and wrote yourself, the bigger your personal library of good and bad examples, and the easier it will be for you to tell what the "best"* way to represent a given concept is.

*Of course it's subjective!

Answer 2

In this case, it looks to me like it would be easier just to change the while condition:

while (numPrime < 10001) {

That's usually the case when a while(true) loop ends with

if (condition)
{
    break;
}

... although you need to check whether anything else in the body of the loop performs a continue.

Alternatively, you could restructure it slightly:

int current = 1;
for (int i = 0; i < 10001; i++)
{
    current++;
    while (!isPrime(current))
    {
        current++;
    }
}

Then current will be the answer at the end.

I generally prefer a for loop over a while loop, when you're trying to do something a particular number of times. In this case the "something" is "find the next prime".

There are various bits of dogma in programming that I feel are taken too far - including "one exit point to a method" and "don't use break." Write code as readably as you can. If you look at some code and feel it's not blindingly obvious what's going on, try to work out other ways of structuring it. Sometimes that's a case of changing a loop; sometimes it's extracting a method; sometimes it's inverting some logic (deal with a negative branch first, possibly exiting early, and then handle the normal case).

Answer 3

if you can get away without a break, then do it.

do

 while (numPrime < 10001) ...

Answer 4

That is what do/while was invented for:

do {
//...
} while(numPrime < 10001);

It's the while(true) bit that I find bad practice, which of course leads to break.

Answer 5

There are a couple of conditions when it makes sense to use a break. One is when you need to execute N.5 loops -- i.e., you'll execute the loop some number of times, but the last time through you'll always execute somewhere in the middle of the loop body. You can avoid using a break in this case, but doing so often obfuscates the code or results in duplication. For example:

while (true) {
    first part;
    if (finished) 
       break;
    second part;
}

can be turned into something like:

first part;
while (!finished) {
    second part;
    first part;
}

or:

while (!finished) {
    first part;
    if (!finished)
        second part;
}

Neither of these is necessarily a major improvement. Another circumstance under which a break can make sense is simply dealing with something like an error. For example, if you've been passed N files to process, it may make sense to break out of the loop if one of them fails to open. Nonetheless, when it's at all reasonable, it's clearly better to have the condition under which you'll exit from the loop explicitly stated in the loop's condition.

Answer 6

As others have noted, the example you gave is a bad one because you could easily move the test into the WHILE.

The cases where I use a break are when I have a loop where I must do some processing before I know that this is the last time through. For example:

while (true)
{
  String inLine=inStream.readLine();
  if (inLine.startsWith("End"))
    break;
  ... process line ...
}

This example is only slightly contrived: I often run into problems where I read some data from a file or other source, parse this data in some way, and only then know that I have reached the end of what I am interested in processing right now.

You could, of course, wrte a function that reads the line and does the parsing necessary, like:

while (!endOfInterestingData(inStream))
{
  ... process ...
}

But then you may have the problem that both the function and the body of the loop need access to the data read, and the function must return a boolean to control loop processing so it can't return the data, so the only way to make the data available to the loop body is to have the function throw it into some mutually-accessible field, which then obscures where the loop gets its data from.

Answer 7

Not hugely different than anything that has been said before, but from readablity, transparent logic stand point I would recommend

long i,primes=0;
for (i=2;primes<10001;i++) {
    if (isPrime(i))
        primes++;
}

i is the answer.

Answer 8

This solution is pretty fast:

#include <stdio.h>

unsigned long isqrt(unsigned long n) {
    // http://snippets.dzone.com/posts/show/2715
    unsigned long a;
    for (a = 0; n >= (2*a)+1; n -= (2*a++) + 1);
    return a;
}

void nPrimes(const long N, long *const primes)
{
    unsigned long n, count, i, root;

    primes[0] = 2;
    count = 1;

    for (n = 3; count < N; n+=2) {

        root = isqrt(n);

        for (i = 0; primes[i] <= root; i++) {
            if ((n % primes[i]) == 0) goto notPrime;
        }
/*      printf("Prime #%lu is: %lu\n", count+1, n);*/
        primes[count++] = n;

        notPrime: ;
    }
}

int main (int argc, char **argv)
{
    long N;

    if (argc > 1) {
        N = atoi(argv[1]);
    } else {
        N = 10001;
    }

    long primes[N];

    nPrimes(N, primes);

    printf("Prime #%lu is: %lu\n", N, primes[N-1]);
}

Notes:

• isqrt(n) is floor(sqrt(n)). This avoids floating-point operations, which we don't need anyway. Algorithm from http://snippets.dzone.com/posts/show/2715.
• nPrimes keeps a list of the primes. This allows you to test only prime divisors, which cuts out the vast majority of those costly mod operations.
• Even the primes are only tested up to isqrt(n).
• That's an ugly goto, but C doesn't have named continues, and a flag variable would have been a complete waste of time.
• The primes array is initialized with 2, and it only checks odd numbers from 3. Eliminating multiples of 3 is similarly doable, but not of trivial complexity.
• The isqrt call could be eliminated by keeping a table of squares of primes. This is almost certainly overkill, but you can if you want to.
• On my not-new machine, this runs in the blink of an eye.