simple two players dice throwing game

I'm a first year uni student. We were asked to come up with code that can represent a simple two players dice throwing game (specifications are given below).

I know as a matter of fact that there are many areas upon which my code could be improved. However, 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 think I could get more out of this exercise this way.

So could someone please provide me with some comments or suggestions? Critics of any level and kind are welcomed! Feel free to pick apart my code!

Game class:

```
import java.util.Scanner;

public class Game {



private Player p1;
private Player p2;
private Dice dice;
private int scoreToWin;

void displayGameMenu() {
System.out.println();
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.print("Choose an option: ");
}

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

void startNewGame() {
String p1Name;
String p2Name;

Scanner sc = new Scanner(System.in);
System.out.print("Please enter player one name: ");
p1Name = sc.nextLine();
System.out.print("Please enter player two name: ");
p2Name = sc.nextLine();
System.out.print("Please

Awkward split of responsibilities

The displayGameMenu method prints a list of options.
This method is called from the main method,
which is also in charge of validating the input.
There are some issues with this:

-
The code of displayGameMenu and main are far away from each other: I have to scroll to see both. This is a problem because when I look at main it's not obvious what number 5 is. And when adding a new menu option, it's easily possible to forget to update one of the relevant locations.

-
The main method should simply call other methods, and have no logic of its own. The main method is designed for execution, not really a functional part of a class. As such, it is awkward that the responsibility of handling the menu is split between a member method (displayGameMenu) and a static method (main)

I suggest to reduce main to something like this:

public static void main(String[] args) {
    new Game().run();
}

And I suggest to eliminate displayGameMenu, the code will be easier to understand if the menu options are easily visible next to the input validation code.

Limit the scope

In this code:

int optionSelected;

while (true) {
    // ...
    optionSelected = sc.nextInt();

The variable optionSelected is not needed outside of the loop.
So it's better to declare it inside:

while (true) {
    // ...
    int optionSelected = sc.nextInt();

Similarly, in playOneRound, you don't need to declare the result variable at the top, it's better to declare it in each of the branches of if-else.

Declare variables right before you need them

In startNewGame you declare p1Name and p2Name at the top.
That's unnecessary. It's better to declare them when you assign them,
for example:

String p1Name = sc.nextLine();

It's ok to reuse Random

In Dice.rollDice, you recreate an instance of Random on every call.
This is unnecessary.
It would be more efficient to store a single instance in private final Random random and reuse it across calls to rollDice.

Naming

A name like setTotalScore(int score) sounds like a classic setter that sets the value of a field to the given value. But this method in Player doesn't set to this value, it appends this to the current value. So addScore would be more natural.

The convention for naming constants is SHOUT_CASE in Java. So numberOfSides in Dice should be named NUMBER_OF_SIDES. Actually I'd just call it simply SIDES.

The word "Dice" in the rollDice method of the Dice class is redundant:
the name of the class already implies that you're rolling a dice, not your eyes.

Pointless local variables

The result variable in Dice.rollDice is really unnecessary and tedious.
The method could be reduced to a single line:

int roll() {
    return random.nextInt(SIDES) + 1;
}

Order of conditions

Instead of this:

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

It might be slightly more readable if the terms are in a consistent order:

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

Another alternative:

while (!(0 <= optionSelected && optionSelected <= 5)) {

Order of modifiers

In private final static int numberOfSides the modifiers are not in the conventional order. The recommended ordering is private static final.

StackExchange Code Review Q#126115, answer score: 5