Playing “craps” for the win v0.3

Continud from Playing “craps” for the win v0.2


You roll two dice. Each die has six faces, which contain one, two,
three, four, five and six spots, respectively. After the dice have
come to rest, the sum of the spots on the two upward faces is
calculated. If the sum is 7 or 11 on the first throw, you win. If the
sum is 2, 3 or 12 on the first throw (called “craps”), you lose (i.e.,
the “house” wins). If the sum is 4, 5, 6, 8, 9 or 10 on the first
throw, that sum becomes your “point.” To win, you must continue
rolling the dice until you “make your point” (i.e., roll that same
point value). You lose by rolling a 7 before making your point.

```
// Craps.java

package co.vu.xxx.craps;

import java.security.SecureRandom;

public class Craps {
public static enum Outcome {UNDEFINED, WIN_FIRST, WIN_POINT, LOSE_FIRST, LOSE_POINT};
private int currentNumber;
private int point;
private Outcome gameResult = Outcome.UNDEFINED;
private static SecureRandom randomNumbers = new SecureRandom();

public static int rollDice() {
int randN = 2 + randomNumbers.nextInt(6) + randomNumbers.nextInt(6);
return randN;
}

public void firstRoll() {
currentNumber = rollDice();
switch (currentNumber) {
case 7:case 11:
gameResult = Outcome.WIN_FIRST;
break;
case 2:case 3:case 12:
gameResult = Outcome.LOSE_FIRST;
break;
default:
point = currentNumber;
}
}

public void moreRolls() {
currentNumber = rollDice();
if(currentNumber == point) {
gameResult = Outcome.WIN_POINT;
} else if(currentNumber == 7) {
gameResult = Outcome.LOSE_POINT;
}
}

public Outcome getGameResult() {
return gameResult;
}

public int getPoint() {
return point;
}

public int getCurrentNumber() {
return currentNumber;

Much of this is nitpicking, but since you keep posting revisions, you seem to want a really thorough review.

• The parameter name pCrap to UseCraps.declareResult() isn't explanatory. Try renaming it to something like game instead.

• I suggest doing the same for crapObj in main().

• getGameResult() should probably be named getState() or something similar, since result implies that it has finished, and there is an UNDEFINED state.

• Speaking of which, the state isn't really undefined, it's just unfinished. It should be renamed to something that doesn't imply an error condition.

• To be super finicky, you could be declaring your local variables randN, currentNumber, crapObj, point, and inputScan as final.

• In Craps.rollDice() you could just return the value instead of storing it in a variable and then immediately returning. The method should also be private.

• UseCraps.declareResult() should be private.

• In UseCraps.waitUser() you don't need to store the line in input since you don't use it.

• Your switch in UseCraps.declareResult() doesn't have a default.

• The Craps class should only expose one roll method. It may make sense to keep the two as separate private ones, but you should simplify what your callers need to do to use your class. Consider what happens if someone calls firstRoll and never calls moreRolls, or if they call moreRolls first, or if they go back and forth.

• Consider adding a reset method to Craps so you can start a new game without having to create a new instance.

• You should check if you are already in a win state when someone calls roll.

StackExchange Code Review Q#68921, answer score: 7