5
\$\begingroup\$

I'm a beginner trying to create a dice game according to the specification given below. This is an improved version of my program based on the feedback given by users on my original question. Again, rather than asking other people to correct my code, I would much prefer to be given hints or general principles as to how my code could be improved. I have been told that there is a hundred way to write a program, a thousand way to break it and a million way to improve it.

Please feel free to pick apart my code and give me any suggestions on areas in my program that could be improved, especially in areas such as code structure and logic.

Program Logic:

The Dice Throwing Game begins with a welcome message followed by a menu with the following options :

Option (1) asks players to enter their names. A player’s name must not be blank (or consists of only spaces and nothing else), but may contain spaces between the characters.

After the names are set up, the game asks for a maximum score. The default maximum score should be set to 200 points. Each player’s initial score is set to 0.

Option (2) simulates the “dice roll” operations for players. When this option is chosen, the computer generates 2 random numbers between 1-6 (ie. simulating a 6-sided dice), representing 2 dice rolls for each player. It then updates players' scores accordingly. The scoring rules for each "round" are as follows :

if the 2 dice rolls have the same value (ie. 1&1, 2&2, …, 6x6), the player scores 2 times the sum of that value (eg. 1&1 scores 4 points, 2&2 scores 8 points, etc) if the 2 dice rolls have different values, the player simply scores the sum of that value (eg. 1&4 scores 5 points, 5&2 scores 7 points, etc) if players reaches a score which is more than the pre-defined maximum, the game’s result is a Draw. note that players can reach over that score at the same time, since for each round, 2 dice rolls are performed for each player, before a winner is decided a player is considered a winner if he accumulates a score which is more than the pre-defined maximum, and the other player has not reached that score Option (3) shows the players current scores, including who is leading the game.

Option (4) displays some brief instructions regarding how to play the game.

Option (5) exits the whole program. All player statistics should be cleared.

Additional Notes :

The menu must be displayed repeatedly after each operation, until the user chooses Option (5). Inputs other than 1-5 should be rejected, with appropriate error messages printed.

If the user chooses Option (2)/(3), before a game has been set up (Option (1)), an appropriate error message should be printed, and the operation aborted.

Your program must deal with invalid values entered by the user in a sensible manner.

Game class:

import java.util.ArrayList;
import java.util.InputMismatchException;
import java.util.List;
import java.util.Scanner;

public class Game {

    List<Player> listOfPlayers = new ArrayList<>();
    private Dice dice = new Dice();
    private int scoreToWin = 200;

    private void run() {

        while (true) {

            System.out.println("(1) Start a new game");
            System.out.println("(2) Play one round");
            System.out.println("(3) Who is leading now?");
            System.out.println("(4) Display game help");
            System.out.println("(5) Exit game");
            System.out.println("Choose an option: ");

            try {

                Scanner sc = new Scanner(System.in);
                int optionSelected = sc.nextInt();

                while (optionSelected < 0 || 5 < optionSelected) {
                    System.out.println("Option entered invalid, please enter a number between 1 - 5");
                    optionSelected = sc.nextInt();
                }

                if (optionSelected == 5) {
                    System.out.println("Exiting...");
                    break;
                }

                this.selectOption(optionSelected);

            } catch (InputMismatchException e) {
                System.out.println("Invalid user input, please enter choose between option 1 - 5");
            }

            if (this.checkIfAnyoneHasWon()) {
                System.out.println("Game ended");
                break;
            }
        }
    }

    private void selectOption(int optionSelected) {
        switch (optionSelected) {
            case 1:
                this.startNewGame();
                break;
            case 2:
                this.playOneRound();
                break;
            case 3:
                this.whoIsLeading();
                break;
            case 4:
                this.displayGameInstruction();
                break;
            default:
                break;
        }
    }

    private void startNewGame() {
        Scanner sc = new Scanner(System.in);
        System.out.println("Would you like to create a player? Enter Y or N.");

        String answer = sc.nextLine();

        while (answer.equalsIgnoreCase("y")) {
            System.out.println("Please enter the player's name.");
            String playerName = sc.nextLine();
            Player newPlayer = new Player(playerName);
            listOfPlayers.add(newPlayer);

            System.out.println("Would like to create another player? Enter Y or N.");
            answer = sc.nextLine();
        }

        System.out.println("Please enter the upper score limit for the game.");
        scoreToWin = sc.nextInt();
    }

    private void playOneRound() {

        for (Player p : listOfPlayers) {
            int currentRoundScore;
            int firstResult = dice.roll();
            int secondResult = dice.roll();

            if (firstResult == secondResult) {
                currentRoundScore = (firstResult + secondResult) * 2;
                p.addToTotalScore(currentRoundScore);
                System.out.format("%s rolled %d and %d, "
                        + "and scored %d points(BONUS DOUBLE POINTS), "
                        + "for a total of %d points.%n",
                        p.getName(), firstResult, secondResult,
                        currentRoundScore, p.getTotalScore());
            } else {
                currentRoundScore = (firstResult + secondResult);
                p.addToTotalScore(currentRoundScore);
                System.out.format("%s rolled %d and %d, "
                        + "and scored %d points, "
                        + "for a total of %d points.%n",
                        p.getName(), firstResult, secondResult,
                        currentRoundScore, p.getTotalScore());
            }
        }
    }

    private void whoIsLeading() {

        List<Player> leaders = new ArrayList<>();

        int highestScore = 0;

        for (Player p : listOfPlayers) {
            if (p.getTotalScore() > highestScore) {
                leaders = new ArrayList<>();
                leaders.add(p);
                highestScore = p.getTotalScore();
            } else if (p.getTotalScore() == highestScore) {
                leaders.add(p);
            }

        }

        for (Player p : leaders) {
            System.out.format("%s is currently leading with "
                    + "%d points.%n", p.getName(), p.getTotalScore());
        }
    }

    private void displayGameInstruction() {
        System.out.println("All players roll a dice twice per turn.");
        System.out.println("If 2 dice rolls have the same value, the player scores 2 times the sum two dice rolls.");
        System.out.println("If 2 dice rolls have different values, the player simply scores the sum of two dice rolls.");
        System.out.println("For each player, result is incremented after each turn.");
        System.out.println("First player to reach or exceed the maxScore wins the game");
    }

    private boolean checkIfAnyoneHasWon() {

        List<Player> winners = new ArrayList<>();

        listOfPlayers.stream().filter((p) -> (p.getTotalScore() >= scoreToWin)).forEach((p) -> {
            winners.add(p);
        });

        winners.stream().forEach((p) -> {
            System.out.printf("%s has reached the game limit of %d for a total "
                    + "of %d points, %s has won the game!%n",
                    p.getName(), scoreToWin,
                    p.getTotalScore(), p.getName());
        });

        return !winners.isEmpty();

    }

    public static void main(String[] args) {
        Game newGame = new Game();
        System.out.println("Welcome to the Dice and roll game!");
        newGame.run();

    }
}

Player class:

public class Player {

    private String name;
    private int totalScore;

    Player(String name){
        this.name = name;
    }

    String getName(){
        return name;
    }

    void setName(){
        this.name = name;
    }  

    void addToTotalScore(int scoreForCurrentRound){
        totalScore += scoreForCurrentRound;
    }

    int getTotalScore(){
        return totalScore;
    }

}

Dice class:

import java.util.Random;

public class Dice {

    private static final int NUMBER_OF_SIDES = 6;

    Random r = new Random();

    int roll(){
         return r.nextInt(NUMBER_OF_SIDES) + 1;
    }
}
\$\endgroup\$
5
\$\begingroup\$

Javadoc, Javadoc, Javadoc


List<Player> listOfPlayers = new ArrayList<>();

players is a completely sufficient name, as it already tells me that it is a collection of some sort. Also, why is this member package private?


while (optionSelected < 0 || 5 < optionSelected) {

That's odd to read, easier to understand is this:

while (optionSelect < 0 || optionSelected > 5) {

while (true) {
    ...
    break;
}

Ideally you would not have an endless loop which you break out from but instead something that tells you if the game is over or not, like this:

while(!isGameOver()) {
    ...
}

if (hasSomebodyWon()) {
    ...
}

private void selectOption(int optionSelected) {

That's quite a bad name for this function, because it does not select an option, it does execute an action. Also I'd argue that the parameters name should be turned around to selectedOption.

Even better would be if you'd represent your options as an enum, that would remove all the magic numbers from your code, but would, of course, add some logic to map read integers to enum values.


while (true) {
    ...
    Scanner sc = new Scanner(System.in);

Why are you constantly recreating the Scanner? Create it ones, use it everywhere.


System.out.println("Would you like to create a player? Enter Y or N.");

String answer = sc.nextLine();

while (answer.equalsIgnoreCase("y")) {
    System.out.println("Please enter the player's name.");
    String playerName = sc.nextLine();
    Player newPlayer = new Player(playerName);
    listOfPlayers.add(newPlayer);

    System.out.println("Would like to create another player? Enter Y or N.");
    answer = sc.nextLine();
}

This could be replaced with a do { ... } while(condition) construct to simplify the code.

do {
    System.out.println("Please enter the player's name.");
    String playerName = sc.nextLine();
    Player newPlayer = new Player(playerName);
    listOfPlayers.add(newPlayer);

    System.out.println("Would you like to create a player? Enter Y or N.");
} while (sc.nextLine().equals("y"))

Assuming that you want at least one player, obviously. You never check if there are enough players, though. Also the players name could be empty.


for (Player p : listOfPlayers) {

Why the unnecessary shortening of the variable?

for (Player player : players) {

Sounds quite repetitive, but is the easiest loop to read.


int currentRoundScore;
int firstResult = dice.roll();
int secondResult = dice.roll();

if (firstResult == secondResult) {
    currentRoundScore = (firstResult + secondResult) * 2;
    p.addToTotalScore(currentRoundScore);
    System.out.format("%s rolled %d and %d, "
            + "and scored %d points(BONUS DOUBLE POINTS), "
            + "for a total of %d points.%n",
            p.getName(), firstResult, secondResult,
            currentRoundScore, p.getTotalScore());
} else {
    currentRoundScore = (firstResult + secondResult);
    p.addToTotalScore(currentRoundScore);
    System.out.format("%s rolled %d and %d, "
            + "and scored %d points, "
            + "for a total of %d points.%n",
            p.getName(), firstResult, secondResult,
            currentRoundScore, p.getTotalScore());
}

This can be simplified to:

int firstResult = dice.roll();
int secondResult = dice.roll();

int currentRoundScore = firstResult + secondResult;

System.out.format("%s rolled %d and %d, ", p.getName(), firstResult, secondResult);

if (firstResult == secondResult) {
    currentRoundScore = currentRoundScore * 2;
    System.out.format("and scored %d points(BONUS DOUBLE POINTS), ", currentRoundScore);
} else {
    System.out.format("and scored %d points ", currentRoundScore);
}

p.AddToTotalScore(currentRoundScore;

System.out.format("for a total of %d points.%n", p.getTotalScore());

And it could be further simplified.


List<Player> leaders = new ArrayList<>();

int highestScore = 0;

for (Player p : listOfPlayers) {
    if (p.getTotalScore() > highestScore) {
        leaders = new ArrayList<>();
        leaders.add(p);
        highestScore = p.getTotalScore();
    } else if (p.getTotalScore() == highestScore) {
        leaders.add(p);
    }

}

Iterating twice would be neater solution. First find the highest score (why aren't you storing that in the game state anyway?) and then find all the players with that score. That way you don't need to create a new list, and especially not just to throw it away again.


private boolean checkIfAnyoneHasWon() {

This function does not only check if anyone has won, it also prints the winner to stdout, the name is misleading.


void setName(){
    this.name = name;
}  

void addToTotalScore(int scoreForCurrentRound){
    totalScore += scoreForCurrentRound;
}

int getTotalScore(){
    return totalScore;
}

Why are these functions package private?


void setName(){
    this.name = name;
}

Should a player really be able to change their name half way through? Or should it keep the name during the whole game?


void addToTotalScore(int scoreForCurrentRound){

Given that there is only one score variable, you might as well call it score and not totalScore.


Random r = new Random();

int roll(){
     return r.nextInt(NUMBER_OF_SIDES) + 1;
}

Why is this package private again?


Random r = new Random();

One word of caution regarding the default constructors of random generators. In this case it is absolutely fine, the internal implementation does its best to make sure that two Random instances are not getting the same seed. However, always check what a default constructor does, in the worst case it seeds the RNG with the current time in milliseconds, which will lead to collisions if multiple RNGs are created in rapid succession (I believe the Random object from .NET had that problem, but I'm not sure).


You have zero checks in place to make sure that the correct order of events is adhered to. I can start the program and tell it to play a round, I can add players in the middle of the game and so on.

A finite state machine is usually used for such simple games. Well, they are even used for complex ones. You want your state machine to first ask for players, then allow to roll dices and then end the game or start a new one.

\$\endgroup\$
  • \$\begingroup\$ Thank you thank you thank you! This is so awesome! Really appreciate your help! \$\endgroup\$ – Thor Apr 26 '16 at 22:34

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.