Mathematical correctness
Mathematical correctness
And if you were aiming to calculate the probabilities of rolling two of five fair dice with the same score, three of five dice with the same score, etc. Then the results are not correct either. Because, for instance, Three of a kind, Four of a king and Yatzy combinations are not treated as a One pair (although, there's at least one pair of dice with the same score in each of them).
YatzyProbabilityCounter isn't really doing what it claims to do. It represents a dice combination, hence a more appropriate name for it would be DiceCombinationYatzyCombination or YatzyCombinationYatzyDiceCombination.
It's not immediately clear what N field in YatzyProbabilityCounter is supposed to represent. It should be something along the lines of MAX_DIE_SCORE.
Method is isParticular(int) is a bit vague, allDiceHaveScore(int) will communicate its purpose more clearly.
The issue is that it performs only a partial update, leaving the object in an inconsistent state with stale data related to the previous combination in the counters array.
To mitigate this, methods isThreeOfKind(), isFullHouse(), isYatzy(), etc. are invoking loadCounterArray() before proceeding with their logic, which is an SRP violation. That's
This approach is error-prone (you might have forgotten to invoke it in one of these 7 methods) and while calculating probabilities you're repeatedly "updating" to the same values already updated counters array by the same values for no good reason.
Similarly, constructor should produce a new combination instance with the fully initialized state (when you construct a new YatzyProbabilityCounter its counters are not evaluated, which should not be the case).
Refactoring suggestions
Firstly, I encourage you to implement suggestions regarding naming and state-management provided above.
If your intent was to calculate the probability of rolling a specific dice combination in a game of Yatzy, which I believe is the case. And a Full house is not supposed to be also considered a One pair and a Three of a kind (i.e. each combination can be characterized by only one type). Then it makes sense to introduce an attribute describing a combination type in the YatzyCombination.
The advantages of doing so:
- It will allow simplifying the class API by hiding checks
isYatzy(), isFullHouse(), etc. and exposing instead an accessor method returning a combination type.
- Making methods
isYatzy(), isFullHouse(), etc. internal gives a safety net while still keeping their logic simple. For instance, isOnePair() might not account for other combinations that contain one pairs (as in your code), the correctness of determining the type of combination will be insured by the order in which these checks are performed. That should happen in one place, in an internal dedicated method, which you can thoroughly unit-test.
Additionally, implementation of combination type checks can be simplified by leveraging Stream API, e.g.
public boolean isYatzy() {
return Arrays.stream(dice).allMatch(d -> d == dice[0]);
}
Remainder: updating counters array is not a responsibility of this method, as explained above it should happen when transitioning to the next combination, not while identifying the combination type.
Now let me outline how YatzyCombination class might look like:
public final class YatzyCombination {
public enum CombinationType {
YATZY, FOUR_OF_KIND, THREE_OF_KIND, TWO_PAIRS, ONE_PAIR,
SMALL_STRAIGHT, LARGE_STRAIGHT, FULL_HOUSE, CHANCE
}
private static final int MAX_DIE_SCORE = 6;
private static final int DICE_NUMBER = 5;
private final int[] dice = new int[DICE_NUMBER];
private final int[] counters = new int[MAX_DIE_SCORE + 1];
private CombinationType type;
public YatzyCombination() {
Arrays.fill(dice, 1);
this.counters[1] = DICE_NUMBER;
this.type = YATZY;
}
public boolean nextCombination() {
if (tryUpdateDice()) { // logic from inc() method
updateCounters(); // logic from loadCounterArray() method
updateCombinationType();
return true;
}
return false;
}
// other methods
}
Where method updateCombinationType() can be implemented as a chain of if-statement performing checks isYatzy(), isFullHouse(), isLargeStraight(), etc. in the order that avoids ambiguity and returning a CombinationType that corresponds to the first matching condition.
Here's a couple observations on further improvement:
- Introducing a "Named constructor"
The no-args constructor exposed by this class creates an instance describing a 11111 yatzy-combination, but new YatzyCombination() doesn't communicate this business intent to the code reader. As an alternative option, we can introduce a static method YatzyCombination.allOnes() and make this constructor private.
Such methods are sometimes called named constructors because they produce an instance of a specific type with certain properties reflected in the method name (as opposed to factory methods, which return an instance of super-type).
Please, don't conflate with CQRS architectural pattern
This principle states that, ideally, a method should either be a query (return a value) or a command (perform a side-effect). Which to some degree overlaps with SRP.
Note, as every design other principle, it's not applicable everywhere. In some cases, following SRP can lead to cleaner design and simple tests. But the cases where applying it doesn't make sense (working with concurrency, building fluent APIs), or doesn't bring much value.
In the code, I preserved the contract of your inc() method, which is superseded by nextCombination(). It both changes stuff and returns a boolean value.
In my opinion, in this case, it's not a big deal, and we can leave it like this. Although, seeing while (combination.nextCombination()) will make pause and bring the method's documentation on the screen.
And it's a fairly simple example to introduce the concept. Here's what we'll get if we split the current implementation into command and query:
public void nextCombination() {
if (!hasNextCombination()) {
throw new IllegalStateException();
}
updateDice(); // only updates state and doesn't return anything
updateCounters();
updateCombinationType();
}
public boolean hasNextCombination() {
return Arrays.stream(dice).anyMatch(d -> d != MAX_DIE_SCORE);
}
