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

Numbers are high, numbers are low. Will you guess the right answer, though?

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

Problem

This question is a follow-up to:

High-Low Guessing Game

  • Now with shiny new graphics in JavaFX.



  • You now have the awesome ability to choose which numbers to guess between.



  • Now with Git Repository!



Main.java

package sample;

import javafx.application.Application;
import javafx.fxml.FXMLLoader;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.stage.Stage;

public class Main extends Application {

    @Override public void start(Stage stage) throws Exception {
        Parent root = FXMLLoader.load(getClass().getResource("highlow_view.fxml"));
        Scene scene = new Scene(root);

        stage.setScene(scene);
        stage.show();
    }

    public static void main(String[] args) {
        launch(args);
    }
}


Controller.java

```
package sample;

import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.input.KeyCode;
import javafx.scene.input.KeyEvent;
import java.util.Random;

public class Controller {
@FXML private Label guessLabel;
@FXML private Label numberGuessLabel;
@FXML private TextField txtAddItem;
@FXML private TextField fromAddItem;
@FXML private TextField toAddItem;
@FXML private Label systemOut;
private static Random rand = new Random();
private boolean isInt;
public int randomNumberFrom = 1;
public int randomNumberTo = 10;
private int numberGuess = 0;
private int theRandomNumber = randomInt(randomNumberFrom, randomNumberTo);

public int getTheRandomNumber() {
return theRandomNumber;
}

public static int randomInt(int from, int to) {
return rand.nextInt(to - from + 1) + from;
}

public void countGuess(int numberIn) {
numberGuess++;
numberGuessLabel.setText("Guess counter:" + numberGuess);
if (numberIn theRandomNumber) {
systemOut.setText("Too high");
} else {
systemOut.setText("Con

Solution

private static Random rand = new Random();


This variable can/should be final

public int randomNumberFrom = 1;
public int randomNumberTo = 10;


In Java it is better practice, if you want to allow access to your variables, to make getters and setters and make the variables themselves private.

private int randomNumberFrom = 1;
private int randomNumberTo = 10;

public int getRandomNumberFrom() {
    return this.randomNumberFrom;
}

public int getRandomNumberTo() {
    return this.randomNumberTo;
}


In this case, since these values are so tightly tied together, you could have one method to set both values:

public void setRandomRange(int from, int to) {
    this.randomNumberFrom = from;
    this.randomNumberTo = to;
}


Ideally, you should also do some validation in this method to ensure that to > from. Like this:

public void setRandomRange(int from, int to) throws IllegalArgumentException {
    if (to <= from) {
        throw new IllegalArgumentException("to must be greater than from");
    }
    this.randomNumberFrom = from;
    this.randomNumberTo = to;
}


numberGuess++;
numberGuessLabel.setText("Guess counter:" + numberGuess);


This variable numberGuess has a bad name. It's not until I read this code that I realize "ok, it counts the number of guesses". It could just as well have been the number you guessed at. guessCount would be a better name.

Your textInt method smells because it modifies the isInt field. It also sets the error message in case it's not a valid number. And it also returns an int.

public int textInt(TextField textField) {
    if(!textField.getText().matches("^\\d+$")) {
        systemOut.setText("Pleas enter a number");
        isInt = false;
        return 0;
    }else{
        isInt = true;
        return Integer.parseInt(textField.getText());
    }
}


One small improvement, because you are not using return values less than 0, is to return -1 as a "special case" to indicate that it is not a valid integer. This would get rid of the isInt variable as a start

public int textInt(TextField textField) {
    if(!textField.getText().matches("^\\d+$")) {
        systemOut.setText("Pleas enter a number");
        return -1;
    }else{
        return Integer.parseInt(textField.getText());
    }
}


Then you just check for value >= 0 instead of isInt.

However, it might be better to use the fact that Integer.parseInt throws a NumberFormatException to do this. I think it might be best if you use the following approach, without using it as a method:

try {
    int value = Integer.parseInt(textField.getText());
    // do something if value is integer
}
catch (NumberFormatException ex) {
    systemOut.setText("Please enter a number");
}


@FXML private void handleEnterPressed(KeyEvent event) {
        if (event.getCode() == KeyCode.ENTER) {
            guessing(textInt(txtAddItem));
        }
    }

    @FXML private void guessButton(ActionEvent action){
        guessing(textInt(txtAddItem));
    }


In this case you can have one method call the other one:

@FXML private void handleEnterPressed(KeyEvent event) {
        if (event.getCode() == KeyCode.ENTER) {
            guessButton(null);
        }
    }

Code Snippets

private static Random rand = new Random();
public int randomNumberFrom = 1;
public int randomNumberTo = 10;
private int randomNumberFrom = 1;
private int randomNumberTo = 10;

public int getRandomNumberFrom() {
    return this.randomNumberFrom;
}

public int getRandomNumberTo() {
    return this.randomNumberTo;
}
public void setRandomRange(int from, int to) {
    this.randomNumberFrom = from;
    this.randomNumberTo = to;
}
public void setRandomRange(int from, int to) throws IllegalArgumentException {
    if (to <= from) {
        throw new IllegalArgumentException("to must be greater than from");
    }
    this.randomNumberFrom = from;
    this.randomNumberTo = to;
}

Context

StackExchange Code Review Q#79014, answer score: 3

Revisions (0)

No revisions yet.