4
\$\begingroup\$

Problem:

Write a program that, given a word, computes the scrabble score for that word.

Code:

import java.util.regex.Pattern;
import java.util.Optional;
import java.util.Map;
import java.util.HashMap;


public class Scrabble {
  private static final Pattern WHITE_SPACE = Pattern.compile("\\s+");
  private static final Map<String, Integer> SCRABBLE_SCORE_MAP = initMap();
  private final String phrase;
  private int score;

  public Scrabble(String phrase) {
    this.phrase = Optional.ofNullable(phrase).orElse("")
        .replaceAll(WHITE_SPACE.toString(), "").toLowerCase();
    score = calculate();
  }

  public int getScore() {
    return score;
  }

  private int calculate() {
    int score = 0;
    if (phrase.isEmpty()) {
      return score;
    }

    for (char chr : phrase.toCharArray()) {
      for (String key : SCRABBLE_SCORE_MAP.keySet()) {
        if (key.indexOf(chr) != -1) {
          score += SCRABBLE_SCORE_MAP.get(key);
        }
      }
    }
    return score;
  }

  private static Map<String, Integer> initMap() {
    Map<String, Integer> map = new HashMap<>();
    map.put("aeioulnrst", 1);
    map.put("dg", 2);
    map.put("bcmp", 3);
    map.put("fhvwy", 4);
    map.put("k", 5);
    map.put("jx", 8);
    map.put("qz", 10);
    return map;
  }
}

Test Suite:

import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

import java.util.Arrays;
import java.util.Collection;

import static org.junit.Assert.assertEquals;

@RunWith(Parameterized.class)
public class ScrabbleScoreTest {

    private String input;
    private int expectedOutput;

    @Parameterized.Parameters
    public static Collection<Object[]> data() {
        return Arrays.asList(new Object[][]{
                {"", 0},
                {" \t\n", 0},
                {null, 0},
                {"a", 1},
                {"f", 4},
                {"street", 6},
                {"quirky", 22},
                {"OXYPHENBUTAZONE", 41},
                {"alacrity", 13},
        });
    }

    public ScrabbleScoreTest(String input, int expectedOutput) {
        this.input = input;
        this.expectedOutput = expectedOutput;
    }

    @Test
    public void test() {
        Scrabble scrabble = new Scrabble(input);

        assertEquals(expectedOutput, scrabble.getScore());
    }
}

Design Discussion:

  • Instead of doing same calculation again and again I cached the score.
  • HashMap seem a fine choice to me since I needed to map letters to score.

Question:

Apart from OOP and encapsulation is there any scope for improvement in efficiency?

Ref:

http://exercism.io/exercises/java/scrabble-score/readme

\$\endgroup\$
  • 1
    \$\begingroup\$ Scrabble has various elements like double word/letter scores, how are you going to deal with them using your cached score approach? \$\endgroup\$ – forsvarir May 18 '16 at 15:00
  • \$\begingroup\$ @forsvarir see the test cases and all those passes. I assume the test cases are requirement and documentation as well, anything beyond that is out of the scope of my code. \$\endgroup\$ – CodeYogi May 19 '16 at 2:27
4
\$\begingroup\$

Some suggestions:

  • Make the score map a HashMap < Character, Integer > so every single rated char is mapped to one integer.
  • Do not use Regexes to remove invalid characters. Simply ignore them.
  • Move the phrase out of the constructor. Make your calculate() method accept the phrase instead to allow re-use of the same Scrabble object.

So in the end you can simplify the score evaluation a lot:

public int calculate(String input) {
    int score = 0;
    for (char c: input.toCharArray()) {
        score += SCRABBLE_SCORE_MAP.getOrDefault(c, 0);
    }
} 

In theory, this should run more performant since there are no Regex transformations, you are not cycling through the whole Map and you are not cycling through the whole String to get the score of a character.

\$\endgroup\$
  • 1
    \$\begingroup\$ "... every ... char is mapped to one integer". Ditto. Hashing a string like "abcdef" and then looking up "c" simply does not, indeed cannot, take advantage of the hash. \$\endgroup\$ – radarbob May 18 '16 at 16:23
1
\$\begingroup\$

As has been said by @domi1819, hashing by individual characters would be better than your current approach. As the test suite seems to have been written as part of the exercise you're limited in how you can change the class and still have the tests run in their current form, which is a pity because the suite is forcing you to construct a class that feels wrong.

If your goal is simply to implement the code required to pass the suite however, I'd remove the phrase member variable of the class. Other than in the initial calculation of the score this isn't used. Instead, change the signature of the calculate method to accept the phrase as an argument instead. Similarly, the calculate method could simply update the score member of the class, rather than returning an integer.

\$\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.