Playing “craps” for the win v0.3

Question

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

---

// UseCraps.java

package co.vu.xxx.craps;

import java.io.IOException;
import java.util.Scanner;

public class UseCraps {
    public static void main(String[] args) throws IOException {
        Craps crapObj = new Craps();
        waitUser();
        crapObj.firstRoll();
        declareResult(crapObj);
        while(crapObj.getGameResult() == Craps.Outcome.UNDEFINED) {
            waitUser();
            crapObj.moreRolls();
            declareResult(crapObj);
        }
    }
    public static void declareResult(Craps pCrap) {
        System.out.printf("You got a %d\n", pCrap.getCurrentNumber());
        int point = pCrap.getPoint();
        if(point != 0) {
            System.out.printf("You need to make your point: %d\n", point);
        }
        switch (pCrap.getGameResult()) {
            case LOSE_FIRST:
                System.out.println("You have lost because of hitting crap in the first round.");
                break;
            case WIN_FIRST:
                System.out.println("You have won, you got a lucky number in the first round!");
                break;
            case LOSE_POINT:
                System.out.println("You have lost, you hit 7 before making your point!");
                break;
            case WIN_POINT:
                System.out.println("You have won, you made your point!");
                break;
            case UNDEFINED:
                System.out.println("Game continues...\n");
                break;
        }
        System.out.println();
    }

    private static void waitUser() {
        Scanner inputScan = new Scanner(System.in);
        System.out.print("Press enter to start rolling: ");
        String input = inputScan.nextLine();
    }
}

Answer 1

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.