7
\$\begingroup\$

I am designing two methods to be used in a Java implementation of Yahtzee.

For those not familiar with the game:

Players take turns rolling dice and acquire points based on the face up values of a roll. 5 6-sided dice are rolled per turn. There are 13 categories that determine a score, which are listed here.

Here is the gist of each method:

  1. Score(category, roll) - takes the name of a category (any data type) and an array (in my code, an array of integers), which represents the face up values of a roll. This method computes the score.

  2. SuggestCategory(roll) - takes an array of integers, which represents the face up values of a roll, and determines the valid Yahtzee categories that will give the maximum score (so this method will probably use the previous method).

Currently, my category parameter is an enum type and the roll is an array of integers. Here are the implementations of the methods:

package yahtzee;

/**
* The YahtzeeUtility class implements the Score(Category, int []) and SuggestedCategory(int []) methods.
* It also contains a private method, ValidateCategoryExists(Category, int []), which validates that a 
* category exists for a given roll.
* 
* @author Robert Blatner
* 
*/
public class YahtzeeUtility {

    /**
     * Validates that a category exists given the values in the roll.
     * 
     * @param category
     *              the name of category as an enum constant
     * @param count
     *              the number of occurrences of values 1 - 6 as an array of integers
     * @return a boolean value corresponding to whether category exists for a given count
     */
    private boolean ValidateCategoryExists(Category category, int [] count){
        assert category != null;
        assert count != null;

        boolean validated = false;


        if( category == Category.THREE_OF_A_KIND){ 

            for(int value : count)
                if(value == 3) validated = true;

        }
        else if( category == Category.FOUR_OF_A_KIND){ // "FourOfAKind"

            for(int value : count)
                if(value == 4) validated = true;

        }
        else if( category == Category.YAHTZEE){ // "AllOfAKind"

            for(int value : count)
                if(value == 5) validated = true;

        }
        else if( category == Category.FULL_HOUSE){ // "FullHouse"

            boolean three = false;
            boolean two = true;

            for(int value : count){
                if(value == 2) two = true;
                else if(value == 3) three = true;
            }

            if (two && three) validated = true;

        }
        else if( category == Category.SMALL_STRAIGHT){ // "Small Straight"
            int seq_cnt = 0;

            for(int value : count){
              if(value > 0){
                  seq_cnt++;
                  if(seq_cnt == 4){
                      validated = true;
                      break;
                  }
              }
              else {
                  seq_cnt = 0;
              } 
            }
        }
        else if( category == Category.LARGE_STRAIGHT){ // "Large Straight"
            int seq_cnt = 0;

            for(int value : count){

              if(value > 0){
                  seq_cnt++;
                  if(seq_cnt == 5){
                      validated = true;
                      break;
                  }
              }
              else {
                  seq_cnt = 0;
              }

            }
        }

        return validated;
    }

    /**
     * Given a category name and an array of values, this method computes a score.
     * 
     * @param category
     *              the name of the category as an enum constant
     * @param roll
     *              an array of integers that represent the face up values of a given roll
     * @return the computed score as an integer
     * @throws NullPointerException
     *              is thrown if either parameter is null
     * @throws IllegalArgumentException
     *              is thrown if value of roll is outside of range 1-6.
     */
    public int Score(Category category, int [] roll) {

        if(roll == null) throw new NullPointerException("Array containing values of roll is null.");
        if(category == null) throw new NullPointerException("Category string parameter is null.");

        // Count face up values (and validate values)
        int [] count = new int[8];
        for(int value : roll){
            if(value < 1 || value > 8) throw new IllegalArgumentException("Value of dice is invalid.");

            count[value - 1] += 1;
        }

        // Select Category (and validate category exists if category is not "Aces" -> "Eights" or "Chance") 
        int score = 0;          
        if(category == Category.ACES)
            score = count[0];

        else if(category == Category.TWOS)
            score = count[1]*2;

        else if(category == Category.THREES)
            score = count[2]*3;

        else if(category == Category.FOURS)
            score = count[3]*4;

        else if(category == Category.FIVES) 
            score = count[4]*5;

        else if(category == Category.SIXES) 
            score = count[5]*6;

        else if(category == Category.SEVENS) 
            score = count[6]*7;

        else if(category == Category.EIGHTS) 
            score = count[7]*8;

        else if(category == Category.THREE_OF_A_KIND || category == Category.FOUR_OF_A_KIND) { 
            if(ValidateCategoryExists(category, count)){
                for(int value : roll)
                    score += value;
            }
            else score = 0;
        }
        else if(category == Category.YAHTZEE) { 
            if(ValidateCategoryExists(category, count)) score = 50;
            else score = 0;

        }
        else if(category == Category.FULL_HOUSE) {
            if(ValidateCategoryExists(category, count)) score = 25;
            else score = 0;

        }
        else if(category == Category.SMALL_STRAIGHT){
            if(ValidateCategoryExists(category, count)) score = 30;
            else score = 0;
        }
        else if(category == Category.LARGE_STRAIGHT){
            if(ValidateCategoryExists(category, count)) score = 40;
            else score = 0;
        }
        else if(category == Category.CHANCE) 
            for(int value : roll)
                score += value;     

        return score;
    }

    /**
     * Given an array of values, this method determines the category(s) that return the highest scores. 
     * 
     * @param roll
     *              an array of integers that represent the face up values of a given roll
     * @return a string array that contains names of suggested categories
     * @throws NullPointerException
     *              is thrown if the integer array argument is null
     * @throws IllegalArgumentException
     *              is thrown if a value in the array of integers parameter is not between 1 and 6.
     */
    public String []SuggestedCategories(int [] roll){

        Category []categories = Category.values();
        int num_categories = categories.length;
        int [] scores = new int[num_categories];

        // Compute Scores for every category
        for(int i = 0; i < num_categories; i++)
            scores[i] = Score(categories[i], roll);

        // Determine max scores
        int count = 0;
        int max = 0;
        for(int score : scores){

            if(score > max) {
                max = score;
                count = 1;
            }
            else if(score == max){
                count++;
            }
        }

        // Consolidate max scores in to a String []
        String [] result = new String[count];
        int result_i = 0;

        for(int i = 0; i < num_categories; i++)
            if( scores[i] == max){
                result[result_i] = categories[i].toString();            
                result_i++;
            }

        return result;
    }

}

Here is the enum implementation for the category parameter:

package yahtzee;

/**
 * Category is an enum class that contains the possible dice game category values. An enum allows 
 * errors to be caught at compile time if a client misspells a category name. The downside is that 
 * an entry will need to be added if a category is added to the game.
 * 
 * @author Robert
 *
 */
public enum Category {
    ACES("Aces"),
    TWOS("Twos"),
    THREES("Threes"),
    FOURS("Fours"),
    FIVES("Fives"),
    SIXES("Sixes"),
    THREE_OF_A_KIND("ThreeOfAKind"),
    FOUR_OF_A_KIND("FourOfAKind"),
    YAHTZEE("Yahtzee"),
    FULL_HOUSE("FullHouse"),
    SMALL_STRAIGHT("SmallStraight"),
    LARGE_STRAIGHT("LargeStraight"),
    CHANCE("Chance");

    private final String name;

    Category(String name){
        this.name = name;       
    }

    @Override
    public String toString(){
        return name;
    }
}

The two methods run without errors. The score method does this:

  1. Counts face up values
  2. Validates a category exists
  3. Computes the value of the score

Maybe I could separate Score() into 3 private methods. Does anyone have suggestions for making the code more readable/organized?

Otherwise, is my variable/method naming okay? Is this a good use case for an enum?

Update:

Yes, I am using Java 8.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some helpful answers. \$\endgroup\$ – SirPython Oct 13 '15 at 0:46
  • \$\begingroup\$ It's not really ACE in dice, it's just one... \$\endgroup\$ – TheCoffeeCup Oct 13 '15 at 1:34
  • \$\begingroup\$ The category for summing ones in Yahtzee is called "Aces". \$\endgroup\$ – Robert Blatner Oct 13 '15 at 1:39
  • \$\begingroup\$ Are you on Java 8? \$\endgroup\$ – h.j.k. Oct 13 '15 at 10:15
  • \$\begingroup\$ @RobertBlatner see my updated answer (I missed your update on Java 8 usage previously)... :) \$\endgroup\$ – h.j.k. Oct 20 '15 at 1:25
1
\$\begingroup\$

Style

  • ValidateCategoryExists(Category, int[])
  • Score(Category, int[])
  • SuggestedCategories(int[])

These can be better renamed as validateCategoryExists/getScore/getSuggestedCategories, to follow Java's standard method naming convention.

Arrays are typically declared as T[], and you have some slight inconsistencies there. Standardizing to T[] should ease readability as well. Also, on the topic of inconsistency, I will also suggest sticking with the use of curly braces { ... } throughout as missing out just one of them may be ripe for a huge mistake/losing an hour of sleep while debugging.

validateCategoryExists

Since this is just one large if-else-if 'ladder', you can consider using a switch, which the Category enum will work nicely with. Do also consider return-ing early.

Since on Java 8, you can even streamline the process slightly by using IntStreams for the simpler comparisons...

private static boolean hasNumber(int[] array, int value) {
    return Arrays.stream(count).anyMatch(i -> i == value);
}

The logic for SMALL_STRAIGHT/LARGE_STRAIGHT can also probably be put into a method as well, since the only variable is the number of sequential non-zero counts.

getScore

Similar to the above section, consider if you want to replace with a switch and/or early return usage.

For the else-if blocks that require calling validateCategoryExists(Category, int[]), you can rewrite them as such:

} else if(category == Category.YAHTZEE) {
    // return early
    return validateCategoryExists(category, count) ? 50 : 0;
    // or
    // score = validateCategoryExists(category, count) ? 50 : 0;
} else if(category == Category.FULL_HOUSE) {
    // ...
} // ...

getSuggestedCategories

The general idea is to:

  1. Create a Map of Category to the score (hint: an EnumMap will be quite useful here).
  2. Find the largest score from the Map's values.
  3. Loop through the Map again to put the required keys into a List<String>, and then the desired String[] return object can be derived by using toArray(T[]):

    return list.toArray(new String[0]);
    

There is an alternative implementation if you are on Java 8. Instead of keying on your Category enum, you can use a TreeMap to key by the scores after groupingBy() the enum values. Getting your result is then just one more call to TreeMap.lastEntry():

TreeMap<Integer, List<Category>> treeMap = 
        EnumSet.allOf(Category.class).stream()
                .collect(Collectors.groupingBy(c -> getScore(c, roll), 
                            TreeMap::new, Collectors.toList()));
return treeMap.lastEntry().getValue().toArray(new String[0]);
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.