Dice-throwing game

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

Javadoc, Javadoc, Javadoc

List 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  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 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 shoul

StackExchange Code Review Q#126742, answer score: 5