Multithreaded HiLo game

Question

For my CS class I have to write a HiLo game using multithreading. I am brand new to multithreading and not sure how to best implement it. The program below works, but I am wondering if there is a better way to do it. When it is run the user will input an int, which is the amount of time that they will have to guess the correct number. If the timer runs out the game will end. I am not supposed to use the Timer object but instead use the System.currentTimeMillis().

import java.util.Random;
import java.util.Scanner;
class Game implements Runnable {
 private static long time;
 private long timer;
 private static long gameTime;
 public Game(int n){
 gameTime=n;
 }
 public void run() {
 time=System.currentTimeMillis();
 while(true){
 timer=(System.currentTimeMillis()-time)/1000;
 if(timer>=gameTime){
 System.out.println("Oops! Time is up - try again.");
 break;
 }
 }
 }
}
public class Hilo {
 public static void main(String[] args) {
 if (args.length!=1){
 System.err.println("Must enter time");
 }
 Random rand = new Random();
 int max=100;
 int min=1;
 int number=rand.nextInt((max-min)+1)+min;
 int gameTime=Integer.parseInt(args[0]);
 System.out.println("Welcome to HiLo!");
 System.out.println("You have "+gameTime+" seconds to guess a number between 1 and 100.");
 Thread clock1 = new Thread(new Game(gameTime));
 clock1.start();
 while(clock1.isAlive()==true){
 System.out.println(">");
 Scanner sc = new Scanner(System.in);
 int input = sc.nextInt();
 if(clock1.isAlive()==true&&input==number){
 System.out.println("You Win!");
 break;
 }else if(clock1.isAlive()==true&&input<number){
 System.out.println("Higher!");
 }else if(clock1.isAlive()==true&&input>number){
 System.out.println("Lower!");
 } 
 }
 }
}

Answer 1

Spacing

First of all, yourcodeispackedverytightly, try using spaces a bit more. This is preferred:

timer = (System.currentTimeMillis() - time) / 1000;
int number = rand.nextInt((max - min) + 1) + min;

---

Static variables

private static long time;
private static long gameTime;

I see absolutely no need to have these as static. As it is right now, your code would get problematic if you instantiated one Game, waited for a while, and then created another game. Each Game should have it's own time and gameTime values.

---

Run and while (true)

public void run() {
 time=System.currentTimeMillis();
 while(true){
 timer=(System.currentTimeMillis()-time)/1000;
 if(timer>=gameTime){
 System.out.println("Oops! Time is up - try again.");
 break;
 }
 }
}

So here you have a repeating loop that does more or less absolutely nothing as fast as it can. As you already know how long you should wait, use Thread.sleep(delay) instead.

---

==true

while(clock1.isAlive()==true){

Using ==true is redundant, simply this is enough:

while (clock1.isAlive()) {

---

Scanner resource

Scanner sc = new Scanner(System.in);

This line should give you a compiler warning because you're not closing the scanner. Consider using a try-with-resources statement.

You only need to create the scanner once, when you start the program. Then you can close it safely at the end of your program.

---

if, if, if...

if(clock1.isAlive()==true&&input==number){
 System.out.println("You Win!");
 break;
}else if(clock1.isAlive()==true&&input<number){
 System.out.println("Higher!");
}else if(clock1.isAlive()==true&&input>number){
 System.out.println("Lower!");
} 

Do you see any common things there? clock1.isAlive() is being checked on every if. Sure, it is possible that it is alive in the first check and then dies before the second if, but that is very unlikely and we're talking about microseconds of difference there. Do the clock1.isAlive check once, and then check compare with the number.

if (clock1.isAlive()) {
 if (input == number) {
 System.out.println("You Win!");
 break;
 } else if(input < number) {
 System.out.println("Higher!");
 } else if(input > number) {
 System.out.println("Lower!");
 }
}

• maaartinus: I'm not sure about closing the Scanner as it would close the underlying System.in, which you didn't allocate. I could imagine using System.in later again, though it mayn't work as the scanner might have eaten some input in its buffer.
• Simon Forsberg: @maaartinus Correct. I just noted that the Scanner is created inside the loop, which is a bad idea. If it is created once on the start of the main method, then it can be closed safely at the end of the main method.
• maaartinus: I missed that. Creating the Scanner inside of the loop is terribly wrong, as it can eat the data to be used for the next iteration. Closing or not, it's broken.

Answer 2

There are a few things I don't like:

• time, timer and gameTime defined as static. Why do you need that fields to be static? That's the recipe for a disaster if you ever need to create multiple instances of your Game class. And even if you just need one instance, please don't declare its member at static. Apart for some very specific cases (like when you define a constant value) you should avoid static variables.
• Thread clock1 = new Thread(new Game(gameTime)); makes me think there is a bit of a confusion in the way you named your Game class. If it is effectively just a timer, why don't you simply call it Timer?

• I'd also change the names of the variables you used in your Game class. What about the following? Note that The class keeps track of duration with a field. The rest can be just local to the scope of run. And usually it is a good idea to give variables the smallest scope possible.

  class Timer implements Runnable {
   private final long duration;
   public Timer(long duration){
   this.duration = duration;
   }
   public void run() {
   long stopTime = System.currentTimeMillis() + duration;
   while(true){
   if(System.currentTimeMillis() >= stopTime){
   // I removed the I/O from here. Let's print the message in your main method
   break;
   }
   }
   }
  }
  

• I'd refactor the while loop to check just once if the timer is alive. Have a look at the other comments I added in my code snippet

  bool isGameOver = false;
  Scanner sc = new Scanner(System.in); // You can reuse the same scanner.
  while(!isGameOver){
   System.out.println(">");
   int input = sc.nextInt();
   if(clock1.isAlive()){ // No need to have the == true part of the condition
   if(input == number){
   System.out.println("You Win!");
   isGameOver = true;
   }
   else if(input<number){
   System.out.println("Higher!");
   }
   else{ // No need to check that input > number here
   System.out.println("Lower!");
   }
   }
   else{
   System.out.println("Oops! Time is up - try again.");
   isGameOver = true;
   }
  }
  

Answer 3

This is on top of the many good points others have already pointed out.

In particular, Game could be a lot cleaner:

• gameTime can be final: it never changes during the lifetime of a Game
• The constructor param n would be better named gameTime
• In the run method:
  • time would be better named start
  • timer would be better named elapsedTime
  • the variable storing elapsed time would be better declared inside the loop
• As Simon pointed out, your loop is a busy loop, which is not good, the CPU spinning and spinning evaluating the check for elapsed time very fast. You could Thread.sleep(gameTime * 1000), but that might go against the instructions of your prof. In that case you can at least sleep for some time, for example 1 second or 100 milliseconds, which would be much better than a busy loop.

With these suggestions applied, the class becomes:

class Game implements Runnable {
 private final long gameTime;
 public Game(int gameTime) {
 this.gameTime = gameTime;
 }
 public void run() {
 long start = System.currentTimeMillis();
 while (true) {
 long elapsedTime = (System.currentTimeMillis() - start) / 1000;
 if (elapsedTime >= gameTime) {
 System.out.println("Oops! Time is up - try again.");
 break;
 }
 try {
 Thread.sleep(1000);
 } catch (InterruptedException e) {
 // another thread has kindly asked your thread to exit
 break;
 }
 }
 }
}

UPDATE

I will quote @Simon's excellent comment about handling the InterruptedException in this example:

If you get an InterruptedException from Thread.sleep, another thread has kindly asked your thread to exit. I think the proper response then is to break from the while-loop, which will end the method, which will end the thread.

• Simon Forsberg: Many good points here. However, if you get an InterruptedException from Thread.sleep, another thread has kindly asked your thread to exit. I think the proper response then is to break from the while-loop, which will end the method, which will end the thread.

Answer 4

The other comments cover the code quite well, I have a few points to add.

First, obviously this is a critical error:

if (args.length!=1){
 System.err.println("Must enter time");
}

And yet you continue with your program knowing that it cannot possibly work. Either throw an exception (which will be reported to the user, and be ugly) or simply return to exit main and terminate the program:

if (args.length!=1){
 System.err.println("Must enter time");
 return;
}

This is a long and confusing way or writing your intent:

 Random rand = new Random();
 int max=100;
 int min=1;
 int number=rand.nextInt((max-min)+1)+min;

min and max should be compile time constants and be at the top of the class:

private static final int MIN_NUM = 1;
private static final int MAX_NUM = 100;

This way they are outside of the running code and clearly constants. Similarly Random should (almost) always by an instance or class variable as having more than one is (almost) never useful.

Use printf for outputting formatted messages:

System.out.printf("You have %s seconds to guess a number between 1 and 100.%n", gameTime);

This leads to fewer string concatenation problems and is easier to read.

Here is where the problems really start:

while(clock1.isAlive()==true){
 out.println(">");
 Scanner sc = new Scanner(System.in);
 int input = sc.nextInt();
 if(clock1.isAlive()==true&&input==number){
 out.println("You Win!");
 break;
 }else if(clock1.isAlive()==true&&input<number){
 out.println("Higher!");
 }else if(clock1.isAlive()==true&&input>number){
 out.println("Lower!");
 }
}

1. never create a Scanner in a loop, and always close resources.
2. never write if(boolean == true) this is ugly and error prone (if(boolean = true) also compiles).
3. you only need to check clock1.isAlive once; after reading user input.

Lets tidy up these simple comments first:

try (final Scanner sc = new Scanner(System.in)) {
 while (true) {
 out.println(">");
 int input = sc.nextInt();
 if(!clock1.isAlive()) {
 break;
 }
 if (input == number) {
 out.println("You Win!");
 break;
 } else if (input < number) {
 out.println("Higher!");
 } else if (input > number) {
 out.println("Lower!");
 }
 }
}

Now we have a number of issues:

1. The clock1 has no idea that we've won - it carries on and still emits that we lost.
2. clock1 is a busy wait, so causes one CPU to "spin" while it's checking the time leaving it unavailable for anything else.

As clock1 runs in a separate Thread, we need to consider visibility and atomicity of any variables used to communicate. So an AtomicBoolean seems the natural choice:

final AtomicBoolean gameOver = new AtomicBoolean(false);
final Thread thread = new Thread(new Runnable() {
 @Override
 public void run() {
 try {
 TimeUnit.SECONDS.sleep(gameTime);
 } catch (InterruptedException e) {
 return;
 }
 gameOver.set(true);
 }
});
thread.start();

So we create a AtomicBoolean gameOver to communicate between the clock Thread and the main Thread. We place a sleep in the clock thread that sleeps for gameTime seconds, if the thread is interrupted before waking, it simply exits. This is much more efficient than your busy wait as it doesn't tie up an entire CPU core for absolutely no reason. Never busy wait!

We can now modify the game loop to:

try (final Scanner sc = new Scanner(System.in)) {
 while (true) {
 out.println(">");
 int input = sc.nextInt();
 if (gameOver.get()) {
 System.out.println("Oops! Time is up - try again.");
 break;
 }
 if (input == number) {
 out.println("You Win!");
 thread.interrupt();
 break;
 } else if (input < number) {
 out.println("Higher!");
 } else if (input > number) {
 out.println("Lower!");
 }
 }
}

So we check whether the game is over after reading a new value from the user. In the winning condition we call thread.interrupt() which interrupts the clock thread and causes it to exit.

• Simon Forsberg: You point out some good things here but there are some things which is incorrect/bad: 1. The spec says that Timer should not be used. 2. I rarely see import static for System.out, I would not recommend that (although I can't really say there is anything explicitly wrong with it). 3. sc.nextInt() is a blocking operation, which is why a check needs to be done after that operation to make sure that the thread is still alive.
• Boris the Spider: @SimonAndréForsberg all very good points - I seem to be being exceptionally slow today. Tried to address those points.
• Simon Forsberg: Much better, definitely worth an upvote.