Playing “craps” for the win v0.2

Question

Continued from Playing "craps" for the win

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.io.IOException;
import java.security.SecureRandom;
import java.util.Scanner;

public class Craps {
    private int point;
    public static enum possibleResults {UNDEFINED, WIN, LOSE};
    protected possibleResults gameResult;
    private String cause;
    private static SecureRandom randomNumbers = new SecureRandom();

    public Craps() {
        gameResult = possibleResults.UNDEFINED;
    }

    public static int rollDice() {
        int randN = 2 + randomNumbers.nextInt(6) + randomNumbers.nextInt(6);
        System.out.printf("Rolling dice... You got %d%n", randN);
        return randN;
    }

    public void firstRoll() throws IOException {
        waitUser();
        int tmp = rollDice();
        switch (tmp) {
            case 7:case  11:
                gameResult = possibleResults.WIN;
                cause= String.format("you have got a lucky number in the first round: %d.", tmp);
                break;
            case 2:case 3:case 12:
                gameResult = possibleResults.LOSE;
                cause= String.format("you have got an unlucky number in the first round: %d.", tmp);
                break;
            default:
                point = tmp;
        }
    }

    public void declareResult() {
        if(point != 0) {
            System.out.printf("You need to make your point: %d%n", point);
        }
        switch (gameResult) {
            case LOSE:
                System.out.printf("You have lost, because %s%n", cause);
                break;
            case WIN:
                System.out.printf("You have won, because %s%n", cause);
                break;
            case UNDEFINED:
                System.out.println("Game continues...\n");
                break;
        }
        System.out.println();
    }

    public void moreRolls() {
        waitUser();
        int tmp = rollDice();
        if(tmp == point) {
            gameResult = possibleResults.WIN;
            cause = "you have made your point!";
        } else if(tmp == 7) {
            gameResult = possibleResults.LOSE;
            cause = "you have hit 7 before making your point.";
        }
    }

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

---

// UseCraps.java

package co.vu.xxx.craps;

import java.io.IOException;

public class UseCraps {
    public static void main(String[] args) throws IOException {
        Craps crapObj = new Craps();
        crapObj.firstRoll();
        crapObj.declareResult();
        while(crapObj.gameResult == Craps.possibleResults.UNDEFINED) {
            crapObj.moreRolls();
            crapObj.declareResult();
        }
    }
}

Answer 1

At a glance, the code looks reasonably good.

Either initialize everything in the constructor:

protected possibleResults gameResult;
private static SecureRandom randomNumbers;

public Craps() {
    this.gameResult = possibleResults.UNDEFINED;
    this.randomNumbers = new SecureRandom();
}

… or get rid of the constructor completely (it is implicit):

protected possibleResults gameResult = possibleResults.UNDEFINED;
private static SecureRandom randomNumbers = new SecureRandom();

---

There is a curious mix of private and protected members. Why? I suggest making them all private unless you see a need to expose them.

The gameResult member should be made private; other classes should have to call a public method getResult() to retrieve it. That prevents other code from setting the gameResult.

---

As an enum, possibleResults should follow the naming conventions for classes (beginning with a capital letter). I suggest shortening the name to

public static enum Outcome { UNDEFINED, WIN, LOSE };

Putting it as the first thing in the Craps class would be good, so as to avoid interrupting the class definition with an enum definition.

Answer 2

Your Craps class should keep its responsibilities limited to playing the game and not printing out and passing around messages. This makes the class more flexible.

To this end, your possibleResults enum should actually have more states describing the different win and loss conditions. You can have states like WIN_NATURAL, WIN_POINT, LOSE_CRAPS, and LOSE_SEVEN. Then these states can be distinguished by the calling class and Craps no longer has to pass around and display messages.

Then you can move all of the message generation to the UseCraps class. This enables any class that wants to use Craps to display messages on the command line, or in a GUI, or however else without binding them to System.out. You should also move the waiting done by waitUser() to UseCraps. Then UseCraps (or any other consumer) can decide how to pause in its event loop (or not pause at all) rather than being restricted to System.in.

Also, everything that @200_success said.