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

Question

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

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 7

If there is only one condition under which a loop can exit, and there isn't a whole lot of code that needs to run before checking the condition, and there isn't a whole lot of code that needs to run after checking the condition, put the condition in the loop.

I do think there are certainly uses for a "do{}while(1);" loop. Among them:

1. In the loop-continue case, the primary condition needs to have enough code run both before and after it is checked that putting the code in the condition itself would be awkward at best.
2. The loop has multiple exit conditions, and the exit condition which would most logically sit at the top or bottom of the loop would require special code to execute which should not execute for other exit conditions.

Some people like to use flags to distinguish exit conditions. I tend to view such flags as an effort to avoid the coding structures which in fact best embody the action of the program.

If one isn't too concerned about speed, one can avoid ever using any form of goto, premature-exit return, or for that matter using more than one while--and that with a simple condition. Simply write one's entire program as a state machine:

void do_whatever(void)
{
  int current_state=1;
  do
  {
    next_state = 0;
    if (current_state == 1)
    {
      do_some_stuff();
      next_state = 2;
    }
    if (current_state == 2)
    {
      do_some_more_stuff();
      next_state = 3;
    }
    ...
    current_state = next_state;
  } while(current_state);
}

There are times such coding is useful (particularly if the "while" can be pulled out of the do_whatever() routine into a loop which runs several similarly-coded routines "simultaneously"). And it's never necessary to use anything like a "goto". But for readability, structured programming constructs are much nicer.

To my mind, to use a flag to exit a loop and then select one of several pieces of code to execute based upon the exit cause is to replace structured code with unstructured code. If within a loop I write

  if (index >= numItems)
  {
    createNewItem(index);
    break;
  }

it's immediately (and locally) clear that the reason I'm creating a new item is because my index exceeded the number of items, and there's no need for me to redundantly test a condition. If instead I loop until I've found something or run out of items, then I'll either have to redundantly test a condition after the loop or add a flag test to every loop iteration.

Answer 8

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 9

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.