24
\$\begingroup\$

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:

The Map that will be scored

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

AI HardPlus scoring

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

AI Nightmare scoring

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

AI Loser scoring

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 creating ScoreConfig
  • ScoreParameters: Interface for allowing scorers to access analyze data and the parameters sent for scoring
  • Scorer: Marker interface for classes that are responsible for applying score, i.e. Scorers and PostScorers
  • ScoreSet: A Map<AbstractScorer, Double> for keeping track of the weights that should be applied to the scorers
  • ScoreStrategy: Interface for providing the list of fields that should be scored
  • ScoreTools.java: Just a couple of utility methods

This code can also be downloaded from its repository on GitHub.

AbstractScorer.java: (31 lines, 993 bytes)

/**
 * Scorer that is responsible to give score to fields
 * 
 * @param <P> Score parameter type
 * @param <F> The type to apply scores to
 */
public abstract class AbstractScorer<P, F> implements Scorer {
    /**
     * Determine if this scorer should apply scores to the fields under the given circumstances.
     * 
     * @param scores Score parameters and analyzes for the scoring
     * @return True to work with the parameters, false to exclude this scorer entirely from the current scoring process
     */
    public boolean workWith(ScoreParameters<P> scores) {
        return true;
    }
    /**
     * Determine the score for the given field and parameters. 
     * @param field Field to score
     * @param scores Parameters and analyzes for the scoring
     * @return The score to give to the field
     */
    public abstract double getScoreFor(F field, ScoreParameters<P> scores);

    @Override
    public String toString() {
        return this.getClass().getSimpleName();
    }
}

FieldScore.java: (99 lines, 2434 bytes)

/**
 * Score container for a specific field.
 *
 * @param <F> The type to apply scores to.
 */
public class FieldScore<F> implements Comparable<FieldScore<F>> {
    private int rank;
    private double score;
    private final F field;
    private final Map<Scorer, Double> specificScorers;
    private double  normalized;

    public FieldScore(F field) {
        this(field, false);
    }
    /**
     * 
     * @param field Field to score
     * @param detailed If true, details about how much score is given from each scorer will be saved
     */
    public FieldScore(F field, boolean detailed) {
        this.field = field;
        this.specificScorers = detailed ? new HashMap<Scorer, Double>() : null;
    }

    void addScore(AbstractScorer<?, F> scorer, double score, double weight) {
        double add = score * weight;
        this.saveScore(scorer, add);
    }

    private void saveScore(Scorer scorer, double score) {
        this.score += score;
        if (scorer != null && specificScorers != null) {
            this.specificScorers.put(scorer, score);
        }
    }

    void setRank(int rank) {
        this.rank = rank;
    }
    void setNormalized(double normalized) {
        this.normalized = normalized;
    }

    @Override
    public int compareTo(FieldScore<F> other) {
        return Double.compare(this.score, other.score);
    }

    public double getScore() {
        return this.score;
    }

    /**
     * Get the field represented by this {@link FieldScore}
     * @return The field that this object contains score for
     */
    public F getField() {
        return this.field;
    }

    void giveExtraScore(PostScorer<?, F> scorer, double bonus) {
        this.saveScore(scorer, bonus);
    }

    /**
     * Get this field's rank.
     * @return The rank score of this field, where 1 is the best rank
     */
    public int getRank() {
        return rank;
    }

    /**
     * Get this field's normalized score
     * @return Normalized score, from 0 to 1.
     */
    public double getNormalized() {
        return this.normalized;
    }

    /**
     * Get detailed information about which scorer gave what score to this field
     * @return Detailed information, or null if this field did not save details
     */
    public Map<Scorer, Double> getScoreMap() {
        return this.specificScorers == null ? null : new HashMap<Scorer, Double>(this.specificScorers);
    }

    @Override
    public String toString() {
        return "(" + this.field + " score " + this.score + ")";
    }
}

FieldScoreProducer.java: (68 lines, 1887 bytes)

/**
 * 
 *
 * @param <P> Score parameter type
 * @param <F> The type to apply scores to
 */
public class FieldScoreProducer<P, F> {
    private final ScoreConfig<P, F> config;

    private boolean detailed;
    private final ScoreStrategy<P, F> scoreStrategy;

    public FieldScoreProducer(ScoreConfig<P, F> config, ScoreStrategy<P, F> strat) {
        this.config = config;
        this.scoreStrategy = strat;
    }

    public FieldScores<P, F> score(P params, Map<Class<?>, Object> analyzes) {
        FieldScores<P, F> scores = new FieldScores<P, F>(params, config, scoreStrategy);
        scores.setAnalyzes(analyzes);
        scores.setDetailed(this.detailed);
        scores.determineActiveScorers();
        scores.calculateScores();
        scores.rankScores();
        scores.postHandle();

        for (PreScorer<P> prescore : config.getPreScorers()) {
            prescore.onScoringComplete();
        }

        return scores;
    }

    public boolean isDetailed() {
        return detailed;
    }
    /**
     * Set whether or not each FieldScore should contain detailed information about how much score the field got from all different scorers (including post scorers)
     * @param detailed True if detailed, false otherwise.
     */
    public void setDetailed(boolean detailed) {
        this.detailed = detailed;
    }

    public Map<Class<?>, Object> analyze(P param) {
        Map<Class<?>, Object> analyze = new HashMap<Class<?>, Object>();
        for (PreScorer<P> preScorers : this.config.getPreScorers()) {
            Object data = preScorers.analyze(param);
            if (data == null) 
                continue; // avoid NullPointerException
            analyze.put(data.getClass(), data);
        }
        return analyze;
    }

    public ScoreConfig<P, F> getConfig() {
        return this.config;
    }

    public FieldScores<P, F> analyzeAndScore(P params) {
        return this.score(params, this.analyze(params));
    }
}

FieldScores.java: (184 lines, 5823 bytes)

/**
 * Class containing scores, information about ranks, analyzes, and which score configuration that was used.
 *
 * @param <P> Score parameter type
 * @param <F> The type to apply scores to
 */
public class FieldScores<P, F> implements ScoreParameters<P> {
    private final ScoreConfig<P, F> config;
    private final Map<F, FieldScore<F>> scores = new HashMap<F, FieldScore<F>>();
    private final P params;
    private final ScoreStrategy<P, F> scoreStrategy;

    private List<AbstractScorer<P, F>> activeScorers;
    private List<List<FieldScore<F>>> rankedScores;
    private Map<Class<?>, Object> analyzes;
    private boolean detailed;

    @SuppressWarnings("unchecked")
    public <E> E getAnalyze(Class<E> clazz) {
        E value = (E) this.analyzes.get(clazz);
        if (value == null)
            throw new NullPointerException("Analyze " + clazz + " not found. Did you forget to add a PreScorer using ScoreConfigFactory.withPreScorer?");
        return value;
    }

    @Override
    public Map<Class<?>, Object> getAnalyzes() {
        return new HashMap<Class<?>, Object>(this.analyzes);
    }

    FieldScores(P params, ScoreConfig<P, F> config, ScoreStrategy<P, F> strat) {
        this.params = params;
        this.config = config;
        this.scoreStrategy = strat;
    }

    @Override
    public ScoreStrategy<P, F> getScoreStrategy() {
        return scoreStrategy;
    }

    /**
     * Call each {@link AbstractScorer}'s workWith method to determine if that scorer is currently applicable
     */
    void determineActiveScorers() {
        activeScorers = new ArrayList<AbstractScorer<P, F>>();

        for (AbstractScorer<P, F> scorer : config.getScorers().keySet()) {
            if (scorer.workWith(this)) {
                activeScorers.add(scorer);
            }
        }
    }

    /**
     * Process the {@link AbstractScorer}s to let them add their score for each field. Uses the {@link ScoreStrategy} associated with this object to determine which fields should be scored.
     */
    void calculateScores() {
        for (F field : this.scoreStrategy.getFieldsToScore(params)) {
            if (!this.scoreStrategy.canScoreField(this, field))
                continue;

            FieldScore<F> fscore = new FieldScore<F>(field, detailed);
            for (AbstractScorer<P, F> scorer : activeScorers) {
                double computedScore = scorer.getScoreFor(field, this);
                double weight = config.getScorers().get(scorer);
                fscore.addScore(scorer, computedScore, weight);
            }
            scores.put(field, fscore);
        }
    }

    /**
     * Call {@link PostScorer}s to let them do their job, after the main scorers have been processed.
     */
    void postHandle() {
        for (PostScorer<P, F> post : this.config.getPostScorers()) {
            post.handle(this);
            this.rankScores(); // Because post-scorers might change the result, re-rank the scores to always have proper numbers.
        }
    }

    @Override
    public P getParameters() {
        return this.params;
    }

    /**
     * Get a List of all the ranks. Each rank is a list of all the {@link FieldScore} objects in that rank
     * @return A list of all the ranks, where the first item in the list is the best rank
     */
    public List<List<FieldScore<F>>> getRankedScores() {
        return rankedScores;
    }

    /**
     * @return A {@link HashMap} copy of the scores that are contained in this object
     */
    public Map<F, FieldScore<F>> getScores() {
        return new HashMap<F, FieldScore<F>>(this.scores);
    }

    /**
     * Get the {@link FieldScore} object for a specific field.
     * @param field Field to get data for
     * @return FieldScore for the specified field.
     */
    public FieldScore<F> getScoreFor(F field) {
        return scores.get(field);
    }

    /**
     * (Re-)calculates rankings for all the fields, and also calculates a normalization of their score
     */
    public void rankScores() {
        SortedSet<Entry<F, FieldScore<F>>> sorted = ScoreTools.entriesSortedByValues(this.scores, true);
        rankedScores = new LinkedList<List<FieldScore<F>>>();
        if (sorted.isEmpty())
            return;
        double minScore = sorted.last().getValue().getScore();
        double maxScore = sorted.first().getValue().getScore();
        double lastScore = maxScore + 1;

        int rank = 0;
        List<FieldScore<F>> currentRank = new LinkedList<FieldScore<F>>();
        for (Entry<F, FieldScore<F>> score : sorted) {
            if (lastScore != score.getValue().getScore()) {
                lastScore = score.getValue().getScore();
                rank++;
                currentRank = new LinkedList<FieldScore<F>>();
                rankedScores.add(currentRank);
            }
            score.getValue().setRank(rank);
            double normalized = ScoreTools.normalized(score.getValue().getScore(), minScore, maxScore - minScore);

            score.getValue().setNormalized(normalized);
            currentRank.add(score.getValue());
        }
    }

    /**
     * Get all {@link FieldScore} objects for a specific rank
     * @param rank From 1 to getRankLength() 
     * @return A list of all FieldScores for the specified rank
     */
    public List<FieldScore<F>> getRank(int rank) {
        if (rankedScores.isEmpty()) return null;
        return rankedScores.get(rank - 1);
    }
    /**
     * Get the number of ranks
     * @return The number of ranks
     */
    public int getRankCount() {
        return rankedScores.size();
    }
    /**
     * @return The score configuration that was used to calculate these field scores.
     */
    public ScoreConfig<P, F> getConfig() {
        return this.config;
    }

    void setAnalyzes(Map<Class<?>, Object> analyzes) {
        this.analyzes = new HashMap<Class<?>, Object>(analyzes);
    }

    /**
     * @param detailed True to store detailed information about which scorer gives which score to which field. False otherwise
     */
    public void setDetailed(boolean detailed) {
        this.detailed = detailed;
    }
}

PostScorer.java: (53 lines, 1779 bytes)

/**
 * A scorer that can apply/modify scores after the regular {@link AbstractScorer}s have done their job.
 *
 * @param <P> Score parameter type
 * @param <F> The type to apply scores to
 */
public abstract class PostScorer<P, F> implements Scorer {
    @Override
    public String toString() {
        return "Post-" + this.getClass().getSimpleName();
    }

    /**
     * Optionally apply any scores to the given {@link FieldScores} object.
     * @param scores The collection of scores to work on.
     */
    public abstract void handle(FieldScores<P, F> scores);

    /**
     * Add score to a field
     * @param fscore {@link FieldScore} container for the field
     * @param score Score to give
     */
    protected void addScore(FieldScore<F> fscore, double score) {
        if (fscore == null) 
            throw new NullPointerException("FieldScore was null.");
        fscore.giveExtraScore(this, score);
    }
    /**
     * Add score to a field
     * @param scores {@link FieldScores} object containing the field
     * @param field Field to apply score for
     * @param score Score to apply
     */
    protected void addScore(FieldScores<P, F> scores, F field, double score) {
        FieldScore<F> fscore = scores.getScoreFor(field);
        this.addScore(fscore, score);
    }
    /**
     * Set score to an exact value for a field
     * @param scores {@link FieldScores} object containing the field
     * @param field Field to apply score for
     * @param score Score to apply
     */
    protected void setScore(FieldScores<P, F> scores, F field, double score) {
        FieldScore<F> fscore = scores.getScoreFor(field);
        if (fscore == null)
            throw new NullPointerException("Field " + field + " does not have any matching FieldScore.");
        fscore.giveExtraScore(this, score - fscore.getScore());
    }
}

PreScorer.java: (18 lines, 549 bytes)

/**
 * Interface for performing analyze work before scorers start scoring.
 * @param <P> Score parameter type
 */
public interface PreScorer<P> {
    /**
     * Perform analyze and return result of analyze
     * @param params The score parameters
     * @return The object that can be retrieved by the scorers
     */
    Object analyze(P params);
    /**
     * Method that can be used to clean-up variables and resources. Called when a {@link FieldScores} object has been fully completed.
     */
    void onScoringComplete();
}

ScoreConfig.java: (43 lines, 1255 bytes)

/**
 * Score Configuration containing instances of {@link PreScorer}, {@link PostScorer} and {@link AbstractScorer}
 *
 * @param <P> Score parameter type
 * @param <F> The type to apply scores to
 */
public class ScoreConfig<P, F> {
    private final ScoreSet<P, F> scorers;
    private final List<PostScorer<P, F>> postScorers;
    private final List<PreScorer<P>> preScorers;

    public ScoreConfig(ScoreConfig<P, F> copy) {
        this(copy.preScorers, copy.postScorers, copy.scorers);
    }

    public ScoreConfig(List<PreScorer<P>> preScorers, List<PostScorer<P, F>> postScorers, ScoreSet<P, F> scorers) {
        this.postScorers = new ArrayList<PostScorer<P,F>>(postScorers);
        this.preScorers = new ArrayList<PreScorer<P>>(preScorers);
        this.scorers = new ScoreSet<P, F>(scorers);
    }

    public List<PostScorer<P, F>> getPostScorers() {
        return postScorers;
    }

    public ScoreSet<P, F> getScorers() {
        return scorers;
    }

    public List<PreScorer<P>> getPreScorers() {
        return preScorers;
    }

    @Override
    public String toString() {
        return "Scorers:{PreScorer: " + preScorers + ", PostScorer: " + postScorers + ", Scorers: " + scorers + "}";
    }
}

ScoreConfigFactory.java: (124 lines, 3601 bytes)

/**
 * Factory class for creating a {@link ScoreConfig}
 * 
 * @param <P> Score parameter type
 * @param <F> The type to apply scores to
 */
public class ScoreConfigFactory<P, F> {
    private ScoreSet<P, F>  scoreSet;
    private final List<PostScorer<P, F>> postScorers;
    private final List<PreScorer<P>> preScorers;

    public static <Params, Field> ScoreConfigFactory<Params, Field> newInstance() {
        return new ScoreConfigFactory<Params, Field>();
    }

    public ScoreConfigFactory() {
        this.scoreSet = new ScoreSet<P, F>();
        this.postScorers = new LinkedList<PostScorer<P, F>>();
        this.preScorers = new LinkedList<PreScorer<P>>();
    }

    public ScoreConfigFactory<P, F> withScoreConfig(ScoreConfig<P, F> config) {
        ScoreConfigFactory<P, F> result = this;

        for (PreScorer<P> pre : config.getPreScorers()) {
            if (!preScorers.contains(pre))
                result = withPreScorer(pre);
        }

        for (PostScorer<P, F> post : config.getPostScorers()) {
            if (!postScorers.contains(post))
                result = withPost(post);
        }

        for (Entry<AbstractScorer<P, F>, Double> scorer : config.getScorers().entrySet()) {
            AbstractScorer<P, F> key = scorer.getKey();
            double value = scorer.getValue();
            if (!scoreSet.containsKey(key))
                result = withScorer(key, value);
            else {
                scoreSet.put(key, value + scoreSet.get(key));
            }
        }

        return result;
    }

    public ScoreConfigFactory<P, F> copy() {
        ScoreConfigFactory<P, F> newInstance = new ScoreConfigFactory<P, F>();
        return newInstance.withScoreConfig(this.build());
    }

    /**
     * Add a scorer to this factory
     * @param scorer Scorer to add
     * @return This factory
     */
    public ScoreConfigFactory<P, F> withScorer(AbstractScorer<P, F> scorer) {
        scoreSet.put(scorer, 1.0);
        return this;
    }
    /**
     * Add a scorer with the specified weight to this factory.
     * @param scorer Scorer to add
     * @param weight Weight that should be applied to the scorer
     * @return This factory
     */
    public ScoreConfigFactory<P, F> withScorer(AbstractScorer<P, F> scorer, double weight) {
        scoreSet.put(scorer, weight);
        return this;
    }
    /**
     * Multiply all current {@link AbstractScorer}s in this factory's {@link ScoreSet} weights by a factor
     * @param value Factor to multiply with
     * @return This factory
     */
    public ScoreConfigFactory<P, F> multiplyAll(double value) {
        ScoreSet<P, F> oldScoreSet = scoreSet;
        scoreSet = new ScoreSet<P, F>();
        for (Map.Entry<AbstractScorer<P, F>, Double> ee : oldScoreSet.entrySet()) {
            scoreSet.put(ee.getKey(), ee.getValue() * value);
        }

        return this;
    }

    /**
     * Add a {@link PostScorer} to this factory.
     * @param post PostScorer to add
     * @return This factory
     */
    public ScoreConfigFactory<P, F> withPost(PostScorer<P, F> post) {
        postScorers.add(post);
        return this;
    }

    /**
     * Create a {@link ScoreConfig} from this factory.
     * @return A {@link ScoreConfig} for the {@link PreScorer}s, {@link PostScorer} and {@link AbstractScorer}s that has been added to this factory.
     */
    public ScoreConfig<P, F> build() {
        return new ScoreConfig<P, F>(this.preScorers, this.postScorers, this.scoreSet);
    }

    /**
     * Add a {@link PreScorer} to this factory
     * @param analyzer PreScorer to add
     * @return This factory
     */
    public ScoreConfigFactory<P, F> withPreScorer(PreScorer<P> analyzer) {
        this.preScorers.add(analyzer);
        return this;
    }



}

ScoreParameters.java: (24 lines, 627 bytes)

/**
 * Interface for retrieving analyzes and parameters that are used for scoring 
 * @param <P> Score parameter type
 */
public interface ScoreParameters<P> {
    /**
     * @param clazz The class to get the analyze for
     * @return The analyze for the specified class, or null if none was found
     */
    <E> E getAnalyze(Class<E> clazz);
    /**
     * @return Parameter object that are used in the scoring
     */
    P getParameters();
    /**
     * @return All available analyze objects
     */
    Map<Class<?>, Object> getAnalyzes();
    ScoreStrategy<P, ?> getScoreStrategy();
}

Scorer.java: (6 lines, 178 bytes)

/**
 * Marker for classes that are a part of scoring things, i.e. {@link PostScorer} and {@link AbstractScorer}.
 */
public interface Scorer { }

ScoreSet.java: (19 lines, 471 bytes)

/**
 * Map of {@link AbstractScorer}s and the weight that should be applied to them.
 *
 * @param <P> Score parameter type
 * @param <F> The type to apply scores to
 */
public class ScoreSet<P, F> extends LinkedHashMap<AbstractScorer<P, F>, Double> {
    private static final long   serialVersionUID    = 5924233965213820945L;

    ScoreSet() {
    }
    ScoreSet(ScoreSet<P, F> copy) {
        super(copy);
    }
}

ScoreStrategy.java: (24 lines, 964 bytes)

/**
 * Responsible for determining which fields to score with the specified params
 * @param <P> Score parameter type
 * @param <F> The type to apply scores to
 */
public interface ScoreStrategy<P, F> {
    /**
     * Determine the collection of fields to score given the specified params
     * @param params Parameter for the scoring
     * @return A collection of fields which to score
     */
    Collection<F> getFieldsToScore(P params);
    /**
     * Determine whether or not scoring of a particular field should be done, allowing {@link PreScorer}s from the analyze to be taken into consideration.
     * @param parameters Parameters, including analyze objects created by {@link PreScorer}s.
     * @param field The field to score or not to score, that is the question.
     * @return True to score field, false to skip scoring of it.
     */
    boolean canScoreField(ScoreParameters<P> parameters, F field);
}

ScoreTools.java: (50 lines, 1571 bytes)

/**
 * Contains methods for common use among scoring classes
 */
public class ScoreTools {

    /**
     * Normalize a value to the range 0..1 (inclusive)
     * @param value Value to normalize
     * @param min The minimum of all values
     * @param range The range of the values (max - min)
     * @return
     */
    public static double normalized(double value, double min, double range) {
        if (range == 0.0) return 0;
        return ((value - min) / range);
    }

    public static <K, V extends Comparable<? super V>> SortedSet<Map.Entry<K, V>> entriesSortedByValues(Map<K, V> map, final boolean descending) {
        SortedSet<Map.Entry<K, V>> sortedEntries = new TreeSet<Map.Entry<K, V>>(
            new Comparator<Map.Entry<K, V>>() {
                @Override
                public int compare(Map.Entry<K, V> e1, Map.Entry<K, V> e2) {
                    int res;
                    if (descending) 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
                }
            }
        );
        sortedEntries.addAll(map.entrySet());
        return sortedEntries;
    }

    private static Random staticRandom = new Random();
    public static <E> E getRandom(List<E> list, Random random) {
        if (list.isEmpty()) return null;
        if (random == null) 
            random = staticRandom;
        return list.get(random.nextInt(list.size()));
    }
}

Usage / Test

An example of how this code can be used is included on GitHub. See the test.net.zomis.aiscores.ttt.TTMain class for an example of how to use this system in a simple Tic-Tac-Toe game.

You can also see the system in action in my TicTacToe Ultimate application. The numbers and colors on the fields show how good the field is according to the scoring that has been applied (green is good, and high numbers is good).

Questions

I'd like to know how well-implemented this code is and how well it is designed. I'd also like to know if there's a cleaner way to solve the getAnalyze-business, as there needs to be a "many-to-many" relationship between an analyze and the scorers.

I am not primarily looking for comments about my formatting and naming, I'm quite happy with those. I am using Java 6 because I need to support Android. I also need to support GWT which makes String.format impossible.

Besides just being able to make decisions for bots, it is also possible to use with Genetic Algorithms and it's possible to apply score to the way another player plays and from this determining how similar a player plays to the AI. I also have some ideas for how to use this system in the future.

There are more code related to this system (for example, a method to analyze + score + return the field(s) that gets the highest score) available on GitHub, but it is not intended to be reviewed (if you want to review it, let me know and I'll post a question about it).

A related questions about the system itself is available at the Computer Science Stack Exchange site.

\$\endgroup\$
13
\$\begingroup\$

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<F, FieldScore<F>> 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 <P, F> 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<P, F> extends LinkedHashMap<AbstractScorer<P, F>, 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<Map.Entry<K, V>>() {
        @Override
        public int compare(Map.Entry<K, V> e1, Map.Entry<K, V> 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 appears that is works, but there is some missing details when it comes to being a multi-threaded application. I see a number of benefits in that direction.

The code is good, and hangs together 'quite well.

\$\endgroup\$
  • \$\begingroup\$ I disagree with proposing IdentityHashMap. Either the key implements equals correctly and then normal HashMap is fine. Or they inherit Object::equals and then HashMap works like IdentityHashMap and that's also fine, since omitting equals is like saying that the default is fine. Or it's broken or that's a different story. +++ IMHO IdentityHashMap should be used only when you really want to ignore potentially existing equals. \$\endgroup\$ – maaartinus Jul 29 '15 at 6:15
9
\$\begingroup\$

I was started to review the code a few days ago and it seems that rolfl was faster. Anyway, some other thoughts including things which are in the github repository.

  1. Build system: It took a while to load the project into Eclipse. There is no .project file nor pom.xml for Maven nor anything else. I suggest you using a build tool, Maven and Gradle are quite popular nowadays. It would help other developers a lot as it could start the application for them (or at least create a jar which can be started an usual java -jar command.)

  2. It was confusing that the test directory contains examples. I would expect unit test there. See also: Maven's Standard Directory Layout - a lot of developers familiar with that.

  3. AbstractScorer could be an interface. Just for a toString and a simple method which return true is rather an abuse of inheritance.

  4. I'd consider a null check here:

    public FieldScore(final F field, final boolean detailed) {
        this.field = field;
    
  5. FieldScores.detailed: I would consider moving this responsibility to another class (probably with a decorator pattern). The flag arguments smells a little bit:

    Clean Code by Robert C. Martin, Flag Arguments, p41:

    Flag arguments are ugly. Passing a boolean into a function is a truly terrible practice. It immediately complicates the signature of the method, loudly proclaiming that this function does more than one thing. It does one thing if the flag is true and another if the flag is false!

  6. This reminds me temporal coupling:

    FieldScores<P, F> scores = new FieldScores<P, F>(params, config, scoreStrategy);
    scores.setAnalyzes(analyzes);
    scores.determineActiveScorers();
    scores.calculateScores();
    scores.rankScores();
    scores.postHandle();
    

    Clean Code by Robert C. Martin, G31: Hidden Temporal Couplings

\$\endgroup\$
  • 1
    \$\begingroup\$ What do you suggest the test directory should be called instead? Would examples be a better name? \$\endgroup\$ – Simon Forsberg Mar 27 '14 at 17:44
  • 1
    \$\begingroup\$ Many thanks for your review, I will take your comments into consideration! ashamed that I forgot to add .project to git \$\endgroup\$ – Simon Forsberg Mar 27 '14 at 17:44
  • \$\begingroup\$ @SimonAndréForsberg: Yes, I'd go with examples. \$\endgroup\$ – palacsint Mar 27 '14 at 18:25
2
\$\begingroup\$

For now, I comment on one part only, which is IMHO dangerous enough to deserve it:

ScoreTools

public static <K, V extends Comparable<? super V>> SortedSet<Map.Entry<K, V>> entriesSortedByValues(Map<K, V> map, final boolean descending) {
    SortedSet<Map.Entry<K, V>> sortedEntries = new TreeSet<Map.Entry<K, V>>(
        new Comparator<Map.Entry<K, V>>() {
            @Override
            public int compare(Map.Entry<K, V> e1, Map.Entry<K, V> e2) {
                int res;
                if (descending) 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
            }
        }
    );
    sortedEntries.addAll(map.entrySet());
    return sortedEntries;
}

Your special fix can easily lead to a map unable to find anything. As rolfl already noted, your Comparator is non-transitive. But it's not antisymmetric and it's not reflexive either. So you're violating every single point of the contract.

Assuming it works now, it can break anytime. Without e.compare(e) returning non zero, there's no way to find an entry, except by using ==, which is an optimization probably included in the current TreeMap version.

When you want such a preserving Comparator, you can do better:

int res;
if (descending) res = e1.getValue().compareTo(e2.getValue());
else res = e2.getValue().compareTo(e1.getValue());
if (res != 0 || e1.equals(e2)) {
    return res;
}

This ensures reflexivity. Now, we don't care about the result, but need something like transitivity and antisymmetry. We use what we have

res = Integer.compare(e1.getKey().hashCode(), e2.getKey().hashCode());
if (res != 0 || e1.equals(e2)) {
    return res;
}

The chances we come that far are low, but non-zero.

res = Integer.compare(e1.getValue().hashCode(), e2.getValue().hashCode());
if (res != 0 || e1.equals(e2)) {
    return res;
}

Now, we have nothing more to compare. Bad luck. You could decide that it's improbable enough and losing some entries is allowed. You could use your ugly hack now. You could also go the hard route like

if (!uniqueIdMap.contains(e1)) {
     uniqueIdMap.put(e1, uniqueIdMap.size());
}
if (!uniqueIdMap.contains(e2)) {
     uniqueIdMap.put(e2, uniqueIdMap.size());
}
return uniqueIdMap.get(e1) - uniqueIdMap.get(e2);

using a

private Map<Map.Entry<K, V>, Integer> uniqueIdMap;

You don't need to be concerned with performance here, as the chances of double hash collision are pretty low. You may want to use a WeakHashMap to avoid memory leaks or ConcurentHashMap with putIfAbsent. You may want to create it lazily as chances it gets needed are low.

In case your objects don't care about equals, you can use Guava Ordering#arbitrary which is such a tie-breaker (and replaces all my lengthy code above).

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