Return to Answer

• 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

• 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

• 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.

• PostScorer is an abstract class, but PreScorer` 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?

• 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?

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

• since when does this make sense?

  public class ScoreSet<P, F> extends LinkedHashMap<AbstractScorer<P, F>, Double>
  

  ???? ;-)

• 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

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

• 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.

• PostScorer is an abstract class, but PreScorer` 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?

• 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?

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

• Since when does this make sense?

  public class ScoreSet<P, F> extends LinkedHashMap<AbstractScorer<P, F>, Double>
  

  ???? ;-)

• 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

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