7
boolean r = false ; int s = 0 ;
while (r == false) ; 
{
    s = getInt() ; 
    if (!(s>=0 && s<=2)) System.out.println ("try again not a valid response") ; 
    else r = true ; 
}

The text never displays itself even when a 3 or a 123 is entered and the loop never terminates. Whats wrong here?

rrk
  • 15,677
  • 4
  • 29
  • 45
David
  • 14,569
  • 34
  • 78
  • 107
  • Also this isn't an infinite loop, so the `infinite-loop` tag doesn't make sense. – Daniel DiPaolo Apr 09 '10 at 20:35
  • 3
    Actually it is an infinite loop. `while (r == false) ;` is an infinite loop doing nothing, since r is initialized to false. – Dave Costa Apr 09 '10 at 20:41
  • 7
    Stop trying to put multiple things on a line. As much as anything, this is what caused your problem. Once you get in a habit of writing clearly, you'll find this kind of thing happens much less. Also set your warnings in eclipse higher--it should have pointed out the empty statement to you. Don't just address this problem, fix the root causes. – Bill K Apr 09 '10 at 20:48
  • D'oh! How sad is it that I post the accepted answer and then proceed to misread the code a second time and put my foot in my mouth thinking it said `while (false)`? – Daniel DiPaolo Apr 09 '10 at 20:50
  • 2
    GAAAAAAAHH! MY EYES!!! Please consider putting some indentation on that if statement. Even the simplest of control statements can be rendered unreadable by cluttering everything in single lines, without indentations and without opening/closing braces. Also, re-work the expression in that if statement so that it doesn't use negation (!) Very rarely you want to use the negation of a composite boolean expression. – luis.espinal Apr 09 '10 at 21:12
  • 1
    Wouldn't it have been simpler to step through this code in a debugger to find the problem? – Kevin Apr 09 '10 at 23:54

7 Answers7

34

You have a semicolon after the condition. When you use braces to specify a block for your while you don't use a semicolon.

Daniel DiPaolo
  • 55,313
  • 14
  • 116
  • 115
9

Remove the ';' after while.

Alex
  • 3,644
  • 2
  • 19
  • 27
8

Others have pointed out the bug, but your code is scary in other ways that will eventually trip you up:

if (!(s>=0 && s<=2)) System.out.println ("try again not a valid response") ; 
else r = true ;

That's bad because you can easily intend more than one statement to run in the case of the if or else clause. Use curly braces and avoid placing conditional statements on a single line:

if (!(s>=0 && s<=2))
{
    System.out.println ("try again not a valid response");
}
else
{
    r = true;
}

It's easier to read and far less likely to introduce hard-to-see bugs.

Jonathon Faust
  • 12,396
  • 4
  • 50
  • 63
  • 2
    I disagree on using curly braces. If it's a single statement, it's a single statement. As a matter of style, I will indent but not use curlies for single statements... however this is a religious battle. – Armstrongest Apr 09 '10 at 20:34
  • 2
    @Atomiton absolutely, I prefer to use them all the time because I'm likely to come back and change it at some point, which is when I'll screw up if I didn't put them there in the first place. At least put them on a different line -- I hope we can agree on that :) – Jonathon Faust Apr 09 '10 at 20:36
  • I never use curly braces for a single statement, and I often come back and add more to it, and I have never accidentally forgotten to add braces and screwed up my control flow... Unless I have been writing python for awhile and then come back to C, that is. – Carson Myers Apr 09 '10 at 20:40
  • i never come back and make that mistake. – David Apr 09 '10 at 20:42
  • 5
    Although I often leave off the braces on a single-line, I can't defend the practice. The fact is, having the braces there costs virtually nothing, even if they somehow threw you off a little, it would be less than a second to glance at it again. On the other hand, leaving them off--say even once in a thousand "if" statements (let's say when you have been writing python for a while and then come back to C) if you happen to screw it up it could take minutes or hours to fix--that's indefensible and pretty clearly wrong. Yet still I leave them off more often than not... – Bill K Apr 09 '10 at 20:45
  • 3
    I love it when people say something like 'I never come back and make that mistake'. Because it may not be you who comes back to the code, and in any case, code written more than a couple of months ago may a well have been written by someone else. – Paul McKenzie Apr 09 '10 at 21:14
  • separate statements on their own line. it's obvious. – Paul McKenzie Apr 09 '10 at 21:15
3

while(r == false)

should be

while(!r)

Despite what everyone else said about the semicolon, that is what I think is wrong with it :)

John Vint
  • 39,695
  • 7
  • 78
  • 108
  • 1
    `while(r == false)` is more explicit and readable. I suppose people could also argue that is should be `while(false == r)`, but I hate that. Anyways, it isn't that he 'should' change it to `!r` because `r` is a boolean so it won't matter either way, meaning that it's just a matter of style. – Carson Myers Apr 09 '10 at 20:42
  • Consistently following the convention `r` and `!r` instead of `r == false` and `r == true` avoids the error of `r = false` and `r = true` (which will NOT cause a compile error in java). If you want to make it more explicit and readable, rename `r` to something like `continue` or `found`. – ILMTitan Apr 09 '10 at 22:35
  • 1
    continue is a keyword so that would make a terrible variable name. You are right, though, things like r and s are terrible variable names. Use a modern IDE with auto-completion and call it something sensible, like done or found. – ajs410 Apr 10 '10 at 01:47
  • 1
    @Myers I disagree. I think r == false is code stink and would never want to see someone using it in the code I am working with. – John Vint Apr 10 '10 at 17:03
3

+1 to Daniel DiPaolo. I thought I'd post a separate answer to provide clarification of why this is the case.

While loops in Java can be written in one of two ways. If there is just one line to the body of the loop, you can write them in a short-hand fashion:

while (true)
    System.out.println("While loop");

This will print out "While loop" on the console until the program ends. The other option is to specify a loop body between braces, as you have done above:

int i = 0;
while (i < 10) {
    System.out.println("i = " + i);
    i++;
}

This will print out "i = 0", "i = 1", ..., "i = 9" each on a separate line.

What the code you posted does is confuse the two. In the short-hand while loop, the Java parser expects to find a statement between the while loop condition and the semi-colon. Because it does not find a statement here, the while loop runs, but does nothing; it has no body. Furthermore, because the loop has no body, there is no opportunity for your variable r to assume a new value; the condition always evaluates to true and the loop never exits.

If you were to negate the condition in the while loop in your example, i.e.,

boolean r = false ; int s = 0 ;
while (r != false) ; 
{
    s = getInt() ; 
    if (!(s>=0 && s<=2)) System.out.println ("try again not a valid response") ; 
    else r = true ; 
}

(note I left the erroneous semicolon in there), you would find that your intended loop body would execute precisely once, as the loop would never run.

alastairs
  • 6,697
  • 8
  • 50
  • 64
3

In addition to other comments, you should also change the if to

if (s < 0 || s > 2)

It's much more understandable this way.

fish
  • 1,050
  • 9
  • 13
1

Unrelated answer, I really really recommend you to follow Sun's style guidelines.

boolean r = false ; 
int s = 0 ;
while (r == false) {
    s = getInt() ; 
    if (!(s>=0 && s<=2)) {
        System.out.println ("try again not a valid response") ; 
    } else {
      r = true ;
    } 
}

You could get rid of the r variable and the if/else condition if you evaluate the result in the loop it self. 

int s = 0;

while( ( s = getInt() ) < 0 || s > 2 ) {
    System.out.println( "Try again, not a valid response");
}
OscarRyz
  • 196,001
  • 113
  • 385
  • 569