A scoring approach to computer opponents

This code is starting to be used within several of my projects, and therefore I thought it's time to get it reviewed.

Description

The most common application for this code is that there is a computer opponent in a game which needs to make a decision based on some parameters. A list of all possible decisions that can be made is created/retrieved somehow (Note that for Real-Time Strategy games this could be a list such as Build A, Build B, Attack C, Explore D, etc). To make the decision, each option is given score from several sources, called Scorers. Each AI is configured to use a set of Scorers with a weight applied to each scorer.

Below is some pictures for how this is currently used within my Minesweeper Flags game (That is Minesweeper in turn-based 2-player mode where the objective is to take the mines and not avoid them). On each possible field to make a move on, there is a number determining the "rank", 1 is the among the best possible moves, 2 is among the second best, and so on (the number of possible ranks depends on the situation and the scorers that is applied)

The map that will be scored:

Rank of fields after having scores applied by "AI HardPlus":

Rank of fields after having scores applied by "AI Nightmare":

Rank of fields after having scores applied by "AI Loser": (has a preference for fields with low mine probability)

Class Summary (740 lines in 13 files, making a total of 21944 bytes)

• AbstractScorer: Abstract class for a normal scorer

• FieldScore: Stores score information for a single field

• FieldScoreProducer: A class that, given a score configuration, can produce FieldScores.

• FieldScores: Stores FieldScore objects for several fields

• PostScorer: Abstract class for applying score to fields when all AbstractScorers have finished scoring

• PreScorer: Interface responsible for analyzing things if needed before the AbstractScorers begin scoring

• ScoreConfig: Score configuration

• ScoreConfigFactory: Factory class for cre

AbstractScorer

-
Are you sure you need the precision of double for scoring? I only ask because I recently had reason to down-convert a system to float because it had many millions of scorable items and the memory saving ended up being significant.

-
toString() It is best practice to put a toString on each and every class you implement. The toString method is there to communicate with yourself (when debugging) and other programmers who unfortunately may need to devour your stack traces. There is no benefit in having a toString on an abstract class. Your toString, giving just the class's simple name, is even worst than the default Object.toString()

FieldScore

• This is an accumulator for the score attached to a field. It could potentially contain multiple scores. I am disappointed that it is not thread-safe. First impression is that it would be a useful optimization to have the fields all scored by different scorers at the same time. This would not be possible with the current FieldScore.

FieldScoreProducer

-
Some documentation here would be nice. I am not certain 'producer' is the way I would describe the class, but then I am not sure what I would recommend instead.

-
The analyze() method has Generics with Object type. This should be resolved with an additional Generic type on the class. Object is a poor declaration, and it indicates a lack of structure.

-
detailed is odd. I can't see why it is not a final field. The setDetailed should be replaced with a flag on the constructor.

FieldScores

-
It is customary to put the Constructors before the methods of the class... (and after static declarations and methods). You have some regular methods before the Constructor.

-
private final Map> scores is a problem. The key of the map is of type F, but there is no indication as to what that F class is. As a result, you have no idea of whether F implements hashCode() and equals() in a way that is compatible with the Map contract. I would recommend that you declare that class as an IdentityHashMap just in case.

-detailed I don't like that it is not final. Also, you have a setDetailed() method but no isDetailed(). It should be set as part of the constructor as a final.

-
you have the word analyzes in various forms throughout the code. I think the right word is analyzers.

-
getAnalyzes() should wrap the return value in the more lightweight Collections.unmodifiableMap(....) rather than creating a new HashMap

-
getScores() ditto

PostScorer

• again with the toString() on an abstract class.

PreScorer

-
PostScorer is an abstract class, butPreScorer is an interface. I would expect them to be mirrors of each other, but they are not.

-
The method onScoringComplete should be something like onPreScoringComplete, right?

-
Analyze should be returning some generic type, not Object.

-
I would expect the methods on PreScorer and PostScorer to be similar to each other, but they are not... at all.

-
Again, I would expect a more complicated signature on the PreScorer... without it, is it doing something wrong?

ScoreConfig



• In all other methods you have been careful to return copies of the stored structures, but in this class you return the basic values. Should the methods use Collections.unmodifiableMap(....) or new HashMap(....) instead?

ScoreConfigFactory

• Why have a factory class and method if the constructors are public anyway?

ScoreSet

-
Since when does this make sense?

public class ScoreSet extends LinkedHashMap, Double>

???? ;-)

ScoreTools

-
The method entriesSortedByValues creates an anonymous Comparator. Comparators are thread-safe and re-entran, and, as a result, it is just a waste of time to create one each time the method is called. Something like:

private static final ASCENDING_VALUE_COMPARATOR = new Comparator>() {
    @Override
    public int compare(Map.Entry e1, Map.Entry e2) {
        int res;
        e1.getValue().compareTo(e2.getValue());
        else res = e2.getValue().compareTo(e1.getValue());
        return res != 0 ? -res : 1; // Special fix to preserve items with equal values
    }
}

-
While in the process of doing that last one above, it appeared to me that the comparators are the wrong way around in there:

if (descending) res = e1.getValue().compareTo(e2.getValue());
else res = e2.getValue().compareTo(e1.getValue());

-
This line is highly suspicious... this makes the comparator non-transitive. With a comparator, compare(a, b) should be the same sign as -compare(b,a), but, in your case, if a.equals(b), then your code will return 1 in both cases.

return res != 0 ? -res : 1; // Special fix to preserve items with equal values

Your code will likely fail with “Comparison method violates its general contract!”

-
getRandom again makes it hard to move to mult-threaded. Consider using java.util.concurrent.ThreadLocalRandom`

Conclusion

Currently it

StackExchange Code Review Q#45088, answer score: 13