2
\$\begingroup\$

Please review my code as I'm wondering if there is a better way. Here is the question I'm trying to solve:

Scrabble Score

Given a word, compute the scrabble score for that word.

Letter Values

You'll need these:

```plain
Letter                           Value
A, E, I, O, U, L, N, R, S, T       1
D, G                               2
B, C, M, P                         3
F, H, V, W, Y                      4
K                                  5
J, X                               8
Q, Z                               10
```

Examples

"cabbage" should be scored as worth 14 points:

  • 3 points for C
  • 1 point for A, twice
  • 3 points for B, twice
  • 2 points for G
  • 1 point for E

And to total:

  • 3 + 2*1 + 2*3 + 2 + 1
  • = 3 + 2 + 6 + 3
  • = 5 + 9
  • = 14

Extensions

  • You can play a double or a triple letter.
  • You can play a double or a triple word.

Here's my answer:

import java.util.HashMap;
import java.util.Map;

class Scrabble {

    private String word;

    Scrabble(String word) {
        this.word = word;
    }

    int getScore() {

        Map<Character, Integer> lettersMap = new HashMap<>();
        String lettersCap = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

        for (int i = 0; i < lettersCap.length(); i++) {
            if (lettersCap.charAt(i) == 'A' || lettersCap.charAt(i) == 'E' ||
                    lettersCap.charAt(i) == 'I' || lettersCap.charAt(i) == 'O' ||
                    lettersCap.charAt(i) == 'O' || lettersCap.charAt(i) == 'U' ||
                    lettersCap.charAt(i) == 'L' || lettersCap.charAt(i) == 'N' ||
                    lettersCap.charAt(i) == 'R' || lettersCap.charAt(i) == 'S' ||
                    lettersCap.charAt(i) == 'T') {

                lettersMap.put(lettersCap.charAt(i), 1);
                lettersMap.put(lettersCap.toLowerCase().charAt(i), 1);
            }

            if (lettersCap.charAt(i) == 'D' || lettersCap.charAt(i) == 'G') {
                lettersMap.put(lettersCap.charAt(i), 2);
                lettersMap.put(lettersCap.toLowerCase().charAt(i), 2);
            }

            if (lettersCap.charAt(i) == 'B' || lettersCap.charAt(i) == 'C' ||
                    lettersCap.charAt(i) == 'M' || lettersCap.charAt(i) == 'P') {
                lettersMap.put(lettersCap.charAt(i), 3);
                lettersMap.put(lettersCap.toLowerCase().charAt(i), 3);
            }

            if (lettersCap.charAt(i) == 'F' || lettersCap.charAt(i) == 'H' ||
                    lettersCap.charAt(i) == 'V' || lettersCap.charAt(i) == 'W' ||
                    lettersCap.charAt(i) == 'Y') {
                lettersMap.put(lettersCap.charAt(i), 4);
                lettersMap.put(lettersCap.toLowerCase().charAt(i), 4);
            }

            if (lettersCap.charAt(i) == 'K') {
                lettersMap.put(lettersCap.charAt(i), 5);
                lettersMap.put(lettersCap.toLowerCase().charAt(i), 5);
            }

            if (lettersCap.charAt(i) == 'J' || lettersCap.charAt(i) == 'X') {
                lettersMap.put(lettersCap.charAt(i), 8);
                lettersMap.put(lettersCap.toLowerCase().charAt(i), 8);
            }

            if (lettersCap.charAt(i) == 'Q' || lettersCap.charAt(i) == 'Z') {
                lettersMap.put(lettersCap.charAt(i), 10);
                lettersMap.put(lettersCap.toLowerCase().charAt(i), 10);
            }

        }

        int totalValue = 0;

        for (int j = 0; j < word.length(); j++) {

            totalValue += lettersMap.get(word.charAt(j));
        }

        return totalValue;
    }

}
\$\endgroup\$
4
\$\begingroup\$

First of all, you should probably create lettersMap from your constructor. You don't need to create it again every time you want to get score for a word.

There are various ways to make the code shorter. The simplest method is to define the scores as an array, in the same order as the letters:

int[] letterScore = {1, 3, 3, 2 ... 4, 10}; 

Then your whole for-cycle gets reduced to

for (int i = 0; i < lettersCap.length(); i++) {
    lettersMap.put(lettersCap.charAt(i), letterScore[i]);
    lettersMap.put(lettersCap.toLowerCase().charAt(i), letterScore[i]);
}

However, it will also make the code less readable.

You could also define every part separately:

char[] score1letters = {'A', 'E', 'I', 'O', … 'T'};
char[] score2letters = {'D', 'G'};
...
char[] score10letters = {'Q', 'Z'}

and for every such array:

for (int i = 0; i < score1letters.length; i++) {
    lettersMap.put(lettersCap.charAt(i), 1);
    lettersMap.put(lettersCap.toLowerCase().charAt(i), 1);
}

To be honest, the most readable version will probably be:

private static final Map<Character, Integer> lettersMap;
static {
    lettersMap = new HashMap<Character, Integer>();
    lettersMap.put('A', 1);
    lettersMap.put('B', 3);
    lettersMap.put('C', 3);
    lettersMap.put('D', 2);
    ...
    lettersMap.put('Y', 4);
    lettersMap.put('Z', 10);
}

To put lowercase chars, you can either get all the keys at the end and copy their values with lowercased key, or, you can leave only uppercase letters there and instead convert the word to uppercase before calculating its score.

\$\endgroup\$
  • \$\begingroup\$ why lettersMap have to be static? \$\endgroup\$ – M Garp Jul 29 '17 at 10:39
  • \$\begingroup\$ @MGarp It doesn't have to be static but it is shared by all instances of Scrabble therefore making it static is the logical thing to do. \$\endgroup\$ – Sulthan Jul 29 '17 at 10:41
  • \$\begingroup\$ okay and if its static then I don't need to put it in the constructor then? \$\endgroup\$ – M Garp Jul 29 '17 at 10:49
  • \$\begingroup\$ @MGarp No, thestatic { ... } initializer is called only once for every class, before even the constructor gets called. \$\endgroup\$ – Sulthan Jul 29 '17 at 11:21
1
\$\begingroup\$

Having a class called Scrabble with a word as a field is not good. This is because the word does not need to be stored for a long time. The word is only needed once for calculating the score, and after that, it is no longer needed. Therefore it is better to make a method that takes the word as a parameter.

Also the class name is not as helpful as possible. It is already good, giving the reader a hint about what it does, but a better name would be ScrabbleScoreCalculator. Here it is:

public class ScrabbleScoreCalculator {

    public int calculateScore(String word) {
        int score = 0;
        for (char letter : word.toCharArray()) {
            score += getLetterScore(letter);
        }
        return score;
    }

}

This is the basic idea of the whole algorithm. We just sum up the scores for each individual letter. How the score for a single letter is calculated is left as a detail for now. It will be implemented by another method very soon.

The English scrabble letters have the benefit that they are all contiguous in the Unicode character set. Therefore their score can be looked up in a simple array, like this:

public class ScrabbleScoreCalculator {

    /* A = 1, B = 3, C = 3, etc. */
    private static final int[] LETTER_SCORES = {
            1, 3, 3, 2, 1, 4, 2, 4, 1, 8,
            5, 1, 3, 1, 1, 3, 10, 1, 1, 1,
            1, 4, 4, 8, 4, 10
    };

    public int calculateScore(String word) { /* see above */ }

    private int getLetterScore(char letter) {
        return LETTER_SCORES[codePoint - 'A'];
    }
}

This code calculates with characters, which is possible in Java and several other programming languages. For example, 'C' - 'A' == 2, since the distance between these letters in the character set is 2.

When you try to call the method calculateScore("Word"), it will throw an exception, since there is no score defined for the letters o, r and d. This is ok, since scrabble is only played with uppercase letters, and the program has to validate the user input before calculating the score.

If you also want to allow lowercase letters (to make the code work with the example word cabbage), you can write it like this:

private int getLetterScore(char letter) {
    char upperCase = Character.toUpperCase(letter);
    return LETTER_SCORES[upperCase - 'A'];
}
\$\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.