Scrabble tile-counting challenge

While reviewing this Scrabble Tile Counter, I decided to give it a try myself. Here is the challenge, from Reddit's /r/dailyprogrammer:

Description


For this challenge we will be using the tile set from the English
edition, which has 100 tiles total. Here's a reference for the
distribution and point value of each
tile.


Each tile will be represented by the letter that appears on it, with
the exception that blank tiles are represented by underscores _.

Input Description


The tiles already in play are inputted as an uppercase string. For
example, if 14 tiles have been removed from the bag and are in play,
you would be given an input like this:

AEERTYOXMCNB_S

Output Description


You should output the tiles that are left in the bag. The list should
be in descending order of the quantity of each tile left in the bag,
skipping over amounts that have no tiles.


In cases where more than one letter has the same quantity remaining,
output those letters in alphabetical order, with blank tiles at the
end.

10: E
9: I
8: A
7: O
5: N, R, T
4: D, L, U
3: G, S
2: F, H, P, V, W
1: B, C, J, K, M, Q, Y, Z, _
0: X

Solution

The main() function takes its input as a command-line argument.

```
import java.util.*;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

/**
* A collection of Scrabble tiles, initially consisting of the 100 tiles in a
* standard Scrabble game, from which tiles can be removed, and the remaining
* counts reported. Tiles may be uppercase 'A' to 'Z', or a '_' to represent
* a blank tile.
*/
public class ScrabbleTileCounter implements Iterable {
private static final int[] START_COUNTS = {
/ A / 9, / B / 2, 2, 4, 12, 2, 3, 2, 9, 1, 1, 4, / M / 2,
/ N / 6, / O / 8, 2, 1, 6, 4, 6, 4, 2, 2, 1, 2, / Z / 1,
/ junk / -1, / junk / -1, / junk / -1, / junk / -1,
/ _ / 2
};

private int[] counts = Arrays.co

private static final int[] START_COUNTS = {
    /* A */ 9, /* B */ 2, 2, 4, 12, 2, 3, 2, 9, 1, 1, 4, /* M */ 2,
    /* N */ 6, /* O */ 8, 2, 1, 6, 4, 6, 4, 2, 2, 1, 2, /* Z */ 1,
    /* junk */ -1, /* junk */ -1, /* junk */ -1, /* junk */ -1,
    /* _ */ 2
};

What are the junk values for in the START_COUNTS array?

They do not represent a valid Scrabble tile, which are defined as letters A through Z with the blank tile _. It may not be clear for all readers that those junk values correspond to the characters that are between Z and _ in the ASCII table. Consider documenting exactly what they are for and why they are initialized with the magic value of -1.

private int[] counts = Arrays.copyOf(START_COUNTS, START_COUNTS.length);

Since we're dealing with an int array, we could also use .clone() to make a new array with the same content, but it may be faster to use copyOf.

public void remove(char c) throws NoSuchElementException {

NoSuchElementException is a runtime exception. Those unchecked exceptions (contrary to checked ones that must be declared in the signature of the method) should not generally be in the signature. Prefer to document it in the Javadoc than add it in the signature of the method.

This pattern is also used in the JDK itself, it is possible to take as example the Stream API introduced in Java 8: limit(maxSize) is documented to throw an IllegalArgumentException if the given maxSize is negative, and this exception is not present in the method signature.

try {
    return this.n;
} finally {
    this.n = // ...;
}

The usage of the try-finally construct here could be surprising. It is in fact not used to deal with resources that ought to be closed, but to make sure that the current value of n is returned, while n is updated to a next value that is the result of some calculation.

I would argue that the try-finally construct should be restricted to dealing with resources, instead of using cleverly its logic like this (because it may not be clear to everyone, and could even really be obscure).

It would be a lot clearer to just keep the old value in a local variable:

public Integer next() {
    int currN = this.n;
    this.n = Arrays.stream(START_COUNTS).filter(x -> x < n).max().orElse(-1);
    return currN;
}

The proposed solution is using a int[] to store the current count for the each Scrabble tiles. It is then using an iterator to iterate over all the possible count of values and get the letters with that count. One of the possible drawback is that this ties implicitely the code into the sorting logic that is used in the problem. If tomorrow, the problem is changed and asks for output in reverse order, then an iterator logic will need to be changed, instead of a sorting logic.

In fact, I would not use an iterator at all. Although this was probably done on purpose with your solution, I would use a Map to keep the current count of character to the character. It wouldn't introduce a real memory overheard since we're not dealing with a lot of values to store, and it would have the great advantage that this data structure could then be sorted properly, independently of the way the count is being built.

Consider the following instead:

Map> result =
    IntStream.range(0, counts.length)
             .boxed()
             .collect(Collectors.groupingBy(
                i -> counts[i],
                () -> new TreeMap<>(Collections.reverseOrder()),
                Collectors.mapping(i -> String.valueOf((char)(i + 'A')), Collectors.toList())
             ));

result.entrySet()
      .stream()
      .map(e -> e.getKey() + ": " + String.join(", ", e.getValue()))
      .forEach(System.out::println);

The first part of the code goes through the counts array and create a Map> with the number of letter left and a String representing all the characters at that count. It uses a TreeMap as backing map to ensure the keys are correctly ordered in descending order.

Then, it is possible to post-process that Map and simply print its content.

StackExchange Code Review Q#132798, answer score: 6