HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Multithreaded HiLo game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
hilogamemultithreaded

Problem

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&&inputnumber){
System.out.println("Lower!");
}
}
}

Solution

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&&inputnumber){
    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("Lower!");
    }
}

Code Snippets

timer = (System.currentTimeMillis() - time) / 1000;
int number = rand.nextInt((max - min) + 1) + min;
private static long time;
private static long gameTime;
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;
        }
    }
}
while(clock1.isAlive()==true){
while (clock1.isAlive()) {

Context

StackExchange Code Review Q#64034, answer score: 11

Revisions (0)

No revisions yet.