Renaming identifiers, documentation. #2643.

src/main/java/org/opentripplanner/common/diff/Difference.java

@@ -15,6 +15,9 @@
 
 package org.opentripplanner.common.diff;
 
+/**
+ * This represents a single difference found between two objects by the ObjectDiffer.
+ */
 public class Difference {
 
     Object a;

src/main/java/org/opentripplanner/common/diff/GenericObjectDiffer.java → src/main/java/org/opentripplanner/common/diff/ObjectDiffer.java

@@ -12,15 +12,15 @@
 
 /**
  * Perform a recursive deep comparison of objects.
- * This is intended only for testing that graph building is reproducible and serialization restores an identical graph.
- * It should be kept simple.
+ * This is intended for testing that graph building is reproducible and serialization restores an identical graph.
+ * It should be kept relatively simple.
  *
- * This class is not threadsafe. It holds configuration and state internally, and should therefore be used for one
- * comparison only and thrown away.
+ * This class is not threadsafe. It holds configuration and state internally. Each instance should therefore be used
+ * for one comparison only and thrown away.
  */
-public class GenericObjectDiffer {
+public class ObjectDiffer {
 
-    private static final Logger LOG = LoggerFactory.getLogger(GenericObjectDiffer.class);
+    private static final Logger LOG = LoggerFactory.getLogger(ObjectDiffer.class);
 
     /**
      * The maximum recursion depth during the comparison.
@@ -53,13 +53,13 @@ public class GenericObjectDiffer {
 
     private int nObjectsCompared = 0;
 
-    /**
-     * If true, execute the whole comparison procedure, even for two identical object references.
-     * Normally these comparisons would be optimized away since they are by definition identical trees of objects.
-     * However this is useful in tests of the comparison system itself - differences should be impossible.
-     */
     private boolean compareIdenticalObjects = false;
 
+    /**
+     * Tell the Differ to execute the whole comparison procedure even when comparing two identical object references.
+     * Normally such comparisons would be optimized away since they are by definition identical trees of objects.
+     * However this is useful in tests of the comparison system itself - there are known to be no differences.
+     */
     public void enableComparingIdenticalObjects() {
         compareIdenticalObjects = true;
     }
@@ -88,6 +88,10 @@ public void useEquals(Class... classes) {
         useEquals.addAll(Arrays.asList(classes));
     }
 
+    /**
+     * This is the main entry point to compare two objects. It's also called recursively for many other
+     * complex objects in the tree.
+     */
     public void compareTwoObjects (Object a, Object b) {
         nObjectsCompared += 1;
         if (compareIdenticalObjects) {
@@ -149,8 +153,8 @@ else if (a instanceof Multimap) {
             compareMaps(new MultimapWrapper((Multimap) a), new MultimapWrapper((Multimap) b));
         }
         else if (a instanceof Collection) {
-            compareCollections((Collection) a, (Collection) b);
             // TODO maybe even generalize this to iterable?
+            compareCollections((Collection) a, (Collection) b);
         }
         else if (classToCompare.isArray()){
             compareArrays(a, b);
@@ -162,8 +166,8 @@ else if (classToCompare.isArray()){
     }
 
     /**
-     * The fallback comparison method - compare every field of the two objects.
-     * Note that the two objects passed in must already be known to be of the same class.
+     * The fallback comparison method - compare every field of two objects of the same class.
+     * Note that the two objects passed in must already be known to be of the same class by the caller.
      */
     private void compareFieldByField (Object a, Object b) {
         Class classToCompare = a.getClass();

src/test/java/org/opentripplanner/common/diff/GenericObjectDifferTest.java → src/test/java/org/opentripplanner/common/diff/ObjectDifferTest.java

@@ -1,17 +1,9 @@
 package org.opentripplanner.common.diff;
 
-import com.google.common.collect.Sets;
 import org.junit.Test;
-import org.opentripplanner.ConstantsForTests;
-import org.opentripplanner.common.TurnRestriction;
 import org.opentripplanner.routing.graph.Graph;
-import org.opentripplanner.routing.graph.Vertex;
-import org.opentripplanner.routing.services.notes.StaticStreetNotesSource;
-import org.opentripplanner.routing.vertextype.IntersectionVertex;
-import org.opentripplanner.routing.vertextype.StreetVertex;
 
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.stream.IntStream;
 
@@ -20,20 +12,20 @@
 import static org.junit.Assert.assertTrue;
 
 /**
- * This test verifies that the GenericObjectDiffer can find differences in nested objects.
+ * This test verifies that the ObjectDiffer can find differences in nested objects.
  * It is expected to automatically perform a recursive, semantic comparison of the two supplied objects.
  */
-public class GenericObjectDifferTest {
+public class ObjectDifferTest {
 
     @Test
     public void testDiff() throws IllegalAccessException {
         {
             // Compare two separate essentially empty graphs.
-            GenericObjectDiffer genericObjectDiffer = new GenericObjectDiffer();
-            genericObjectDiffer.ignoreFields("graphBuilderAnnotations", "streetNotesService", "vertexById", "buildTime", "modCount");
+            ObjectDiffer objectDiffer = new ObjectDiffer();
+            objectDiffer.ignoreFields("graphBuilderAnnotations", "streetNotesService", "vertexById", "buildTime", "modCount");
             Graph graph1 = new Graph();
             Graph graph2 = new Graph();
-            genericObjectDiffer.compareTwoObjects(graph1, graph2);
+            objectDiffer.compareTwoObjects(graph1, graph2);
         }
 
         // Ideally we'd compare two separate but identical complex graphs.
@@ -49,17 +41,17 @@ public void testDiff() throws IllegalAccessException {
             // No differences should be found between the two.
             Map<Integer, Fields> o1 = makeFieldsMap();
             Map<Integer, Fields> o2 = makeFieldsMap();
-            GenericObjectDiffer genericObjectDiffer = new GenericObjectDiffer();
-            genericObjectDiffer.compareTwoObjects(o1, o2);
-            assertFalse(genericObjectDiffer.hasDifferences());
+            ObjectDiffer objectDiffer = new ObjectDiffer();
+            objectDiffer.compareTwoObjects(o1, o2);
+            assertFalse(objectDiffer.hasDifferences());
 
             // Now make one of the two nested objects different. Differences should be found.
             o2.get(10).nestedMap.get("50").clear();
             // This should fail
-            genericObjectDiffer = new GenericObjectDiffer(); // TODO add a reset function
-            genericObjectDiffer.compareTwoObjects(o1, o2);
-            assertTrue(genericObjectDiffer.hasDifferences());
-            genericObjectDiffer.printDifferences();
+            objectDiffer = new ObjectDiffer(); // TODO add a reset function
+            objectDiffer.compareTwoObjects(o1, o2);
+            assertTrue(objectDiffer.hasDifferences());
+            objectDiffer.printDifferences();
         }
 
     }

src/test/java/org/opentripplanner/routing/graph/GraphSerializationTest.java

@@ -6,7 +6,7 @@
 import org.jets3t.service.io.TempFile;
 import org.junit.Test;
 import org.opentripplanner.ConstantsForTests;
-import org.opentripplanner.common.diff.GenericObjectDiffer;
+import org.opentripplanner.common.diff.ObjectDiffer;
 import org.opentripplanner.common.geometry.HashGridSpatialIndex;
 import org.opentripplanner.routing.impl.DefaultStreetVertexIndexFactory;
 import org.opentripplanner.routing.trippattern.Deduplicator;
@@ -26,16 +26,11 @@
  * Tests that saving a graph and reloading it (round trip through serialization and deserialization) does not corrupt
  * the graph, and yields exactly the same data.
  *
- * Candidates for performing the comparison included:
- * DeepEquals from https://github.com/jdereg/java-util
- * isEqualToComparingFieldByFieldRecursively() from http://joel-costigliola.github.io/assertj/
+ * We tried several existing libraries to perform the comparison but nothing did exactly what we needed in a way that
+ * we could control precisely.
  *
- * We need to use a system that can detect cycles, and one that can display or record any differences encountered.
- * DeepEquals cannot record what differences it encounters. isEqualToComparingFieldByFieldRecursively finds
- * differences but there are so many it runs out of memory.
- *
- * So currently I'm using the object differ supplied by csolem via the Entur OTP branch at
- * https://github.com/entur/OpenTripPlanner/tree/protostuff_poc
+ * The object differ we're using started out as a copy of the one supplied by csolem via the Entur OTP branch at
+ * https://github.com/entur/OpenTripPlanner/tree/protostuff_poc but has been mostly rewritten at this point.
  *
  * Created by abyrd on 2018-10-26
  */
@@ -57,13 +52,13 @@ public void testRoundTrip () throws Exception {
         // We can exclude relatively few classes here, the two trees are of course perfectly identical.
         // We do skip edge lists - otherwise this does a depth-first search of the graph causing a stack overflow.
         // And also some deeply buried weak-value hash maps, which refuse to tell you what their keys are.
-        GenericObjectDiffer genericObjectDiffer = new GenericObjectDiffer();
-        genericObjectDiffer.ignoreFields("incoming", "outgoing");
-        genericObjectDiffer.ignoreClasses(WeakValueHashMap.class);
-        genericObjectDiffer.enableComparingIdenticalObjects();
-        genericObjectDiffer.compareTwoObjects(originalGraph, originalGraph);
-        genericObjectDiffer.printDifferences();
-        assertFalse(genericObjectDiffer.hasDifferences());
+        ObjectDiffer objectDiffer = new ObjectDiffer();
+        objectDiffer.ignoreFields("incoming", "outgoing");
+        objectDiffer.ignoreClasses(WeakValueHashMap.class);
+        objectDiffer.enableComparingIdenticalObjects();
+        objectDiffer.compareTwoObjects(originalGraph, originalGraph);
+        objectDiffer.printDifferences();
+        assertFalse(objectDiffer.hasDifferences());
 
         // Now round-trip the graph through serialization.
         File tempFile = TempFile.createTempFile("graph", "pdx");
@@ -72,19 +67,19 @@ public void testRoundTrip () throws Exception {
 
         // This time we need to make some exclusions because some classes are inherently transient or contain
         // unordered lists we can't yet compare.
-        genericObjectDiffer = new GenericObjectDiffer();
+        objectDiffer = new ObjectDiffer();
         // Skip incoming and outgoing edge lists. These are unordered lists which will not compare properly.
         // The edges themselves will be compared via another field, and the edge lists are reconstructed after deserialization.
-        genericObjectDiffer.ignoreFields("incoming", "outgoing");
-        genericObjectDiffer.useEquals(BitSet.class, LineString.class, Polygon.class);
+        objectDiffer.ignoreFields("incoming", "outgoing");
+        objectDiffer.useEquals(BitSet.class, LineString.class, Polygon.class);
         // HashGridSpatialIndex contains unordered lists in its bins. This is rebuilt after deserialization anyway.
         // The deduplicator in the loaded graph will be empty, because it is transient and only fills up when items
         // are deduplicated.
-        genericObjectDiffer.ignoreClasses(HashGridSpatialIndex.class, ThreadPoolExecutor.class, Deduplicator.class);
+        objectDiffer.ignoreClasses(HashGridSpatialIndex.class, ThreadPoolExecutor.class, Deduplicator.class);
 
-        genericObjectDiffer.compareTwoObjects(originalGraph, copiedGraph);
-        assertFalse(genericObjectDiffer.hasDifferences());
-        genericObjectDiffer.printDifferences();
+        objectDiffer.compareTwoObjects(originalGraph, copiedGraph);
+        assertFalse(objectDiffer.hasDifferences());
+        objectDiffer.printDifferences();
     }
 
 }