Java implementation of Yahtzee

Question

I'm trying to implement the game of Yahtzee in java. This is for a project at my university so I tried to be as clean and to use the best code practices as possible. The part in particular that I would like some suggestion on is the one dedicated on keeping the scores (ScoreCategory, ScoreTable and their implementation). I found myself stuck using lots of magic numbers for indexing the table and also this is not convenient when testing/playing the game. Can you help me figure out a good way to refactor this?

This is the interface for each score category:

public interface ScoreCategory {
    /**
     * @return The name of the category.
     */
    String getName();

    /**
     * This function will calculate the score of the category based on an array of dices values.
     * @param dicesValues The value of the dices.
     * @return The score of the dice values in this category.
     */
    default int calculateScore(int[] dicesValues) {
        return 0;
    };

    /**
     * This function will calculate the score of the category. It will be used for categories where score
     * does not depend on the value of the dices.
     * @return The score of this category.
     */
    default int calculateScore() {
        return 0;
    };

    /**
     * Score the category with the dices values provided.
     * @param dicesValues The value of the dices.
     */
    default void score(int[] dicesValues) {};

    /**
     * Score the category. This is to be used when category score does not depend on the dices values.
     */
    default void score() {};

    /**
     * @return The current score in this category.
     */
    int getScore();

    /**
     * @return True if the category has already been scored once. Otherwise, False.
     * If the category can't be manually scored then this should always return true.
     */
    boolean isScored();
}

And here is how I implemented the table:

public class DefaultScoringTable implements ScoringTable {

    private static final int CATEGORYCOUNT = 17;
    ScoreCategory[] scoringArray = new ScoreCategory[CATEGORYCOUNT];

    public DefaultScoringTable(){
        //0: # of 1
        scoringArray[0] = new GeneralCategory("# of 1", DefaultScoringFunctions.SUM1);
        //1: # of 2
        scoringArray[1] = new GeneralCategory("# of 2", DefaultScoringFunctions.SUM2);
        //2: # of 3
        scoringArray[2] = new GeneralCategory("# of 3", DefaultScoringFunctions.SUM3);
        //3: # of 4
        scoringArray[3] = new GeneralCategory("# of 4", DefaultScoringFunctions.SUM4);
        //4: # of 5
        scoringArray[4] = new GeneralCategory("# of 5", DefaultScoringFunctions.SUM5);
        //5: # of 6
        scoringArray[5] = new GeneralCategory("# of 6", DefaultScoringFunctions.SUM6);

        //6: Sum of above
        scoringArray[6] = new SumOfUpperCategory("Sum of upper", this);
        //7: Bonus
        scoringArray[7] = new BonusCategory("Bonus", this);

        //8: Couple
        scoringArray[8] = new GeneralCategory("Couple", DefaultScoringFunctions.COUPLE);
        //9: Double couple
        scoringArray[9] = new GeneralCategory("Double couple", DefaultScoringFunctions.DOUBLECOUPLE);
        //10: Tris
        scoringArray[10] = new GeneralCategory("Tris", DefaultScoringFunctions.TRIS);
        //11: Poker
        scoringArray[11] = new GeneralCategory("Poker", DefaultScoringFunctions.POKER);
        //12: Small scale
        scoringArray[12] = new GeneralCategory("Small scale", DefaultScoringFunctions.SMALL);
        //13: Big scale
        scoringArray[13] = new GeneralCategory("Big scale", DefaultScoringFunctions.BIG);
        //14: Full
        scoringArray[14] = new GeneralCategory("Full", DefaultScoringFunctions.FULL);
        //15: Sum
        scoringArray[15] = new GeneralCategory("Sum", DefaultScoringFunctions.SUM);
        //16: Yahtzee
        scoringArray[16] = new GeneralCategory("Yahtzee", DefaultScoringFunctions.YAHTZEE);
    }

    @Override
    public int getTotalScore() {
        return Arrays.stream(scoringArray).mapToInt(ScoreCategory::getScore).sum() - scoringArray[6].getScore();
    }

    @Override
    public ScoreCategory getScoringCategory(int index) {
        return scoringArray[index];
    }

    @Override
    public void score(int index, int[] dicesValues) {
        scoringArray[index].score(dicesValues);
    }

    @Override
    public boolean isComplete() {
        for (var category: scoringArray) {
            if (!category.isScored()) return false;
        }
        return true;
    }

    @Override
    public int getCategoryCount() {
        return CATEGORYCOUNT;
    }
}

If you need to look at other parts of the code here is the github repository.

Let me know what you think!

Answer 1

Interface default methods

An interface is meant to define an interface, a list of methods that the implementing class has to provide. Putting implementation into the interface definition (using default methods) violates that concept, so there has to be a good reason to do so, which I don't see in your case.

Even if there's a valid reason for using default methods, then they should do real useful work that (at least potentially) applies to all implementing classes. Your default methods do nothing.

Scoring Array

Make that a List instead of an array. Then you get rid of the ugly CATEGORYCOUNT constant and the indexing during the initialization, by using e.g.

scoringArray.add(new GeneralCategory("# of 1", DefaultScoringFunctions.SUM1));

ScoreCategory

Well in line with the Yahtzee rules, you have different classes implementing this interface (for a more useful review, you should have shown us these classes as well).

An interface is meant to decsribe functionality that is present in all implementing classes, so that user code can treat all ScoreCategory versions the same. So, e.g.

/**
 * Score the category. This is to be used when category score does not depend on the dices values.
 */
default void score() {};

should not be part of the interface, as (according to your documentation) it is only meaningful for some specific categories, forcing the caller to decide whether to use tha method or not.

What's common to scoring categories is that (in isolation) they return a score for a given roll (int[] dicesValues). That should be the core scoring method in the interface:

/**
 * Score the category with the dices values provided.
 * @param dicesValues The value of the dices.
 */
void score(int[] dicesValues);

Callers should only use that method, and let the implementing class decide what to do with or without the dicesValues.

Around that, Yahtzee allows the player to use some categories only once, or to modify the result on the second or third application. I'd build that logic around the ScoreCategory implementations, not into it.

But your isScored() methods implies that you want to integrate this logic into the categories. If we want to go that way, I'd revise the scoring method then: the score not only depends on the current roll, but also on the categories that have been used for scoring so far:

/**
 * Score the category, based on the current situation 
 * (current roll and categories used so far).
 * @param dicesValues The value of the dices.
 * @param categoriesUsedSoFar The categories that have been used so far.
 */
void score(int[] dicesValues, Collection<ScoreCategory> categoriesUsedSoFar);

If I understand the Yahtzee rules correctly, depending on the roll and the scoring done so far, there are situations where you

• cannot score a given category (e.g. has already been used),
• can score it (e.g. has not been used),
• can score it in addition to another category (a second Yahtzee),
• must score it (the matching SUMx category in some second-Yahtzee situations).

I'd cover that applicability with an enum

enum ScoreApplicability { CANNOT, CAN, ADDITION, MUST }

and an interface method

/**
 * Return the applicability mode of this category in the currect situation.
 * @param dicesValues The value of the dices.
 * @param categoriesUsedSoFar The categories that have been used so far.
 * @return applicability
 */
ScoreApplicability applicability(int[] dicesValues, Collection<ScoreCategory> categoriesUsedSoFar);