12
\$\begingroup\$

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 low");
        } else if (numberIn > theRandomNumber) {
            systemOut.setText("Too high");
        } else {
            systemOut.setText("Congratz you were right!");
        }
    }

    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());
        }
    }

    public void guessing(int numberIn){
        if (isInt == true) {
            if (numberIn < randomNumberFrom || numberIn > randomNumberTo) {
                systemOut.setText("Pleas enter number: " + randomNumberFrom + "-" + randomNumberTo);
            } else {
                countGuess(numberIn);
            }
        }
    }

    @FXML public void getNewNumber(ActionEvent action){
        int fromInt = textInt(fromAddItem);
        if (isInt == true) {
            int toInt = textInt(toAddItem);
            if (isInt == true && fromInt < toInt) {
                numberGuess = 0;
                numberGuessLabel.setText("Guess counter:" + numberGuess);
                theRandomNumber = randomInt(fromInt, toInt);
                randomNumberFrom = fromInt;
                randomNumberTo = toInt;
                guessLabel.setText("Guess a number:" + fromInt + "-" + toInt);
            } else{
                systemOut.setText("The second number must be bigger then the first");
            }
        }
    }

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

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

highlow_view.fxml

<?xml version="1.0" encoding="UTF-8"?>
<?import javafx.scene.control.*?>
<?import javafx.scene.layout.*?>
<?import javafx.scene.text.*?>

<AnchorPane id="AnchorPane" prefHeight="200.0" prefWidth="500.0" xmlns:fx="http://javafx.com/fxml" fx:controller="sample.Controller">
<children>
    <VBox fx:id="VBoxMain" alignment="TOP_CENTER" prefHeight="500.0" prefWidth="400.0" AnchorPane.bottomAnchor="40.0" AnchorPane.leftAnchor="100.0" AnchorPane.rightAnchor="100.0" AnchorPane.topAnchor="20.0">
        <children>
            <Label fx:id="systemOut" text="Hello :D" alignment="CENTER" prefHeight="200.0">
                <font>
                    <Font name="System Bold" size="18.0" />
                </font>
            </Label>
            <Label  fx:id="numberGuessLabel" text="Guess counter:0" />
            <Label  fx:id="guessLabel" text="Guess a number:1-10" />
            <TextField fx:id="txtAddItem"  prefWidth="200.0" onKeyPressed="#handleEnterPressed" />
            <HBox id="HBox" fx:id="HBox4Btns" alignment="CENTER" spacing="5.0">
                <children>
                    <Button mnemonicParsing="false" onAction="#guessButton" text="Guess"/>
                    <Button mnemonicParsing="false" onAction="#getNewNumber" text="New Number"/>
                    <TextField fx:id="fromAddItem"  prefWidth="50.0"/>
                    <Label  fx:id="fromToo" text="-" />
                    <TextField fx:id="toAddItem"  prefWidth="50.0"/>
                </children>
            </HBox>
        </children>
    </VBox>
</children>
</AnchorPane>
\$\endgroup\$
  • 2
    \$\begingroup\$ do you have a question? \$\endgroup\$ – blurfus Jan 29 '15 at 21:01
  • 2
    \$\begingroup\$ @ochi Not really. What I want is a general review. \$\endgroup\$ – Henrik Bohlin Jan 29 '15 at 21:03
3
\$\begingroup\$
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);
        }
    }
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.