Some suggestions for task-21095 and beyond:task-21095-suggestions

 * have a comparator class
 * use constants for field names, see org.torproject.onionoo.docs.Fields
   for the example with first_seen and consensus_weight.
 * copyright year needs to be adapted
 * use java 7 throughout: mostly diamond operator
Open:
 * add SummaryComparatorTest class (maybe) using parametrized testing
   in order to make more thorough tests of the ordering mechanism,
   this will also accomodate furture ordering extensions more easily.

5 files changed, 98 insertions, 66 deletions

diff --git a/src/main/java/org/torproject/onionoo/docs/Fields.java b/src/main/java/org/torproject/onionoo/docs/Fields.java
new file mode 100644
index 0000000..ec1f6fa
--- /dev/null
+++ b/src/main/java/org/torproject/onionoo/docs/Fields.java
@@ -0,0 +1,18 @@
+/* Copyright 2017 The Tor Project
+ * See LICENSE for licensing information */
+
+package org.torproject.onionoo.docs;
+
+/** Provides constants for document field names. */
+public interface Fields {
+
+  public static final String FIRST_SEEN = "first_seen";
+  public static final String FIRST_SEEN_ASC = FIRST_SEEN;
+  public static final String FIRST_SEEN_DES = "-" + FIRST_SEEN;
+
+  public static final String CONSENSUS_WEIGHT = "consensus_weight";
+  public static final String CONSENSUS_WEIGHT_ASC = CONSENSUS_WEIGHT;
+  public static final String CONSENSUS_WEIGHT_DES = "-" + CONSENSUS_WEIGHT;
+
+}
+
diff --git a/src/main/java/org/torproject/onionoo/docs/SummaryComparator.java b/src/main/java/org/torproject/onionoo/docs/SummaryComparator.java
new file mode 100644
index 0000000..7715a71
--- /dev/null
+++ b/src/main/java/org/torproject/onionoo/docs/SummaryComparator.java
@@ -0,0 +1,46 @@
+/* Copyright 2017 The Tor Project
+ * See LICENSE for licensing information */
+
+package org.torproject.onionoo.docs;
+
+import java.util.Comparator;
+
+public class SummaryComparator implements Comparator<SummaryDocument> {
+
+  private final String[] orderParameters;
+
+  /** Comparator is initialized with the order parameters. */
+  public SummaryComparator(String ... orderParameters) {
+    this.orderParameters = orderParameters;
+  }
+
+  @Override
+  public int compare(SummaryDocument o1, SummaryDocument o2) {
+    int result = 0;
+    for (String orderParameter : orderParameters) {
+      switch (orderParameter) {
+        case Fields.CONSENSUS_WEIGHT_ASC:
+          result = Long.compare(o1.getConsensusWeight(),
+              o2.getConsensusWeight());
+          break;
+        case Fields.CONSENSUS_WEIGHT_DES:
+          result = Long.compare(o2.getConsensusWeight(),
+              o1.getConsensusWeight());
+          break;
+        case Fields.FIRST_SEEN_ASC:
+          result = Long.compare(o1.getFirstSeenMillis(),
+              o2.getFirstSeenMillis());
+          break;
+        case Fields.FIRST_SEEN_DES:
+          result = Long.compare(o2.getFirstSeenMillis(),
+              o1.getFirstSeenMillis());
+          break;
+        default:
+      }
+      if (result != 0) {
+        break;
+      }
+    }
+    return result;
+  }
+}
diff --git a/src/main/java/org/torproject/onionoo/server/RequestHandler.java b/src/main/java/org/torproject/onionoo/server/RequestHandler.java
index 4c4b63a..9ed3c4e 100644
--- a/src/main/java/org/torproject/onionoo/server/RequestHandler.java
+++ b/src/main/java/org/torproject/onionoo/server/RequestHandler.java
@@ -1,10 +1,11 @@
-/* Copyright 2011--2016 The Tor Project
+/* Copyright 2011--2017 The Tor Project
  * See LICENSE for licensing information */
 
 package org.torproject.onionoo.server;
 
 import org.torproject.onionoo.docs.DocumentStore;
 import org.torproject.onionoo.docs.DocumentStoreFactory;
+import org.torproject.onionoo.docs.SummaryComparator;
 import org.torproject.onionoo.docs.SummaryDocument;
 
 import java.util.ArrayList;
@@ -134,11 +135,9 @@ public class RequestHandler {
     this.family = family;
   }
 
-  private Map<String, SummaryDocument> filteredRelays =
-      new HashMap<String, SummaryDocument>();
+  private Map<String, SummaryDocument> filteredRelays = new HashMap<>();
 
-  private Map<String, SummaryDocument> filteredBridges =
-      new HashMap<String, SummaryDocument>();
+  private Map<String, SummaryDocument> filteredBridges = new HashMap<>();
 
   /** Handles this request by filtering by all given parameters and then
    * possibly ordering, offsetting, and limiting results. */
@@ -191,7 +190,7 @@ public class RequestHandler {
       return;
     }
     boolean runningRequested = this.running.equals("true");
-    Set<String> removeRelays = new HashSet<String>();
+    Set<String> removeRelays = new HashSet<>();
     for (Map.Entry<String, SummaryDocument> e
         : filteredRelays.entrySet()) {
       if (e.getValue().isRunning() != runningRequested) {
@@ -201,7 +200,7 @@ public class RequestHandler {
     for (String fingerprint : removeRelays) {
       this.filteredRelays.remove(fingerprint);
     }
-    Set<String> removeBridges = new HashSet<String>();
+    Set<String> removeBridges = new HashSet<>();
     for (Map.Entry<String, SummaryDocument> e
         : filteredBridges.entrySet()) {
       if (e.getValue().isRunning() != runningRequested) {
@@ -224,7 +223,7 @@ public class RequestHandler {
   }
 
   private void filterBySearchTerm(String searchTerm) {
-    Set<String> removeRelays = new HashSet<String>();
+    Set<String> removeRelays = new HashSet<>();
     for (Map.Entry<String, SummaryDocument> e
         : filteredRelays.entrySet()) {
       String fingerprint = e.getKey();
@@ -277,7 +276,7 @@ public class RequestHandler {
     for (String fingerprint : removeRelays) {
       this.filteredRelays.remove(fingerprint);
     }
-    Set<String> removeBridges = new HashSet<String>();
+    Set<String> removeBridges = new HashSet<>();
     for (Map.Entry<String, SummaryDocument> e :
         filteredBridges.entrySet()) {
       String hashedFingerprint = e.getKey();
@@ -357,7 +356,7 @@ public class RequestHandler {
     } else {
       Set<String> relaysWithCountryCode =
           this.nodeIndex.getRelaysByCountryCode().get(countryCode);
-      Set<String> removeRelays = new HashSet<String>();
+      Set<String> removeRelays = new HashSet<>();
       for (String fingerprint : this.filteredRelays.keySet()) {
         if (!relaysWithCountryCode.contains(fingerprint)) {
           removeRelays.add(fingerprint);
@@ -384,7 +383,7 @@ public class RequestHandler {
     } else {
       Set<String> relaysWithAsNumber =
           this.nodeIndex.getRelaysByAsNumber().get(asNumber);
-      Set<String> removeRelays = new HashSet<String>();
+      Set<String> removeRelays = new HashSet<>();
       for (String fingerprint : this.filteredRelays.keySet()) {
         if (!relaysWithAsNumber.contains(fingerprint)) {
           removeRelays.add(fingerprint);
@@ -408,7 +407,7 @@ public class RequestHandler {
     } else {
       Set<String> relaysWithFlag = this.nodeIndex.getRelaysByFlag().get(
           flag);
-      Set<String> removeRelays = new HashSet<String>();
+      Set<String> removeRelays = new HashSet<>();
       for (String fingerprint : this.filteredRelays.keySet()) {
         if (!relaysWithFlag.contains(fingerprint)) {
           removeRelays.add(fingerprint);
@@ -423,7 +422,7 @@ public class RequestHandler {
     } else {
       Set<String> bridgesWithFlag = this.nodeIndex.getBridgesByFlag().get(
           flag);
-      Set<String> removeBridges = new HashSet<String>();
+      Set<String> removeBridges = new HashSet<>();
       for (String fingerprint : this.filteredBridges.keySet()) {
         if (!bridgesWithFlag.contains(fingerprint)) {
           removeBridges.add(fingerprint);
@@ -460,7 +459,7 @@ public class RequestHandler {
   private void filterNodesByDays(
       Map<String, SummaryDocument> filteredNodes,
       SortedMap<Integer, Set<String>> nodesByDays, int[] days) {
-    Set<String> removeNodes = new HashSet<String>();
+    Set<String> removeNodes = new HashSet<>();
     for (Set<String> nodes : nodesByDays.headMap(days[0]).values()) {
       removeNodes.addAll(nodes);
     }
@@ -480,7 +479,7 @@ public class RequestHandler {
       /* Not filtering by contact information. */
       return;
     }
-    Set<String> removeRelays = new HashSet<String>();
+    Set<String> removeRelays = new HashSet<>();
     for (Map.Entry<String, Set<String>> e :
         this.nodeIndex.getRelaysByContact().entrySet()) {
       String contact = e.getKey();
@@ -503,7 +502,7 @@ public class RequestHandler {
       /* Not filtering by relay family. */
       return;
     }
-    Set<String> removeRelays = new HashSet<String>(
+    Set<String> removeRelays = new HashSet<>(
         this.filteredRelays.keySet());
     removeRelays.remove(this.family);
     if (this.nodeIndex.getRelaysByFamily().containsKey(this.family)) {
@@ -531,38 +530,8 @@ public class RequestHandler {
     }
     if (this.order != null) {
       final String[] orderParameters = this.order;
-      Comparator<SummaryDocument> comparator =
-          new Comparator<SummaryDocument>() {
-        @Override
-        public int compare(SummaryDocument o1, SummaryDocument o2) {
-          int result = 0;
-          for (String orderParameter : orderParameters) {
-            switch (orderParameter) {
-              case "consensus_weight":
-                result = Long.compare(o1.getConsensusWeight(),
-                    o2.getConsensusWeight());
-                break;
-              case "-consensus_weight":
-                result = Long.compare(o2.getConsensusWeight(),
-                    o1.getConsensusWeight());
-                break;
-              case "first_seen":
-                result = Long.compare(o1.getFirstSeenMillis(),
-                    o2.getFirstSeenMillis());
-                break;
-              case "-first_seen":
-                result = Long.compare(o2.getFirstSeenMillis(),
-                    o1.getFirstSeenMillis());
-                break;
-              default:
-            }
-            if (result != 0) {
-              break;
-            }
-          }
-          return result;
-        }
-      };
+      Comparator<SummaryDocument> comparator
+          = new SummaryComparator(orderParameters);
       Collections.sort(uniqueRelays, comparator);
       Collections.sort(uniqueBridges, comparator);
     }
@@ -604,15 +573,13 @@ public class RequestHandler {
     }
   }
 
-  private List<SummaryDocument> orderedRelays =
-      new ArrayList<SummaryDocument>();
+  private List<SummaryDocument> orderedRelays = new ArrayList<>();
 
   public List<SummaryDocument> getOrderedRelays() {
     return this.orderedRelays;
   }
 
-  private List<SummaryDocument> orderedBridges =
-      new ArrayList<SummaryDocument>();
+  private List<SummaryDocument> orderedBridges = new ArrayList<>();
 
   public List<SummaryDocument> getOrderedBridges() {
     return this.orderedBridges;
diff --git a/src/main/java/org/torproject/onionoo/server/ResourceServlet.java b/src/main/java/org/torproject/onionoo/server/ResourceServlet.java
index 8760d2b..8d2fcec 100644
--- a/src/main/java/org/torproject/onionoo/server/ResourceServlet.java
+++ b/src/main/java/org/torproject/onionoo/server/ResourceServlet.java
@@ -1,8 +1,9 @@
-/* Copyright 2011--2016 The Tor Project
+/* Copyright 2011--2017 The Tor Project
  * See LICENSE for licensing information */
 
 package org.torproject.onionoo.server;
 
+import org.torproject.onionoo.docs.Fields;
 import org.torproject.onionoo.util.Time;
 import org.torproject.onionoo.util.TimeFactory;
 
@@ -66,13 +67,13 @@ public class ResourceServlet extends HttpServlet {
 
   private static final long CACHE_INTERVAL = 5L * 60L * 1000L;
 
-  private static Set<String> knownParameters = new HashSet<String>(
+  private static Set<String> knownParameters = new HashSet<>(
       Arrays.asList(("type,running,search,lookup,fingerprint,country,as,"
           + "flag,first_seen_days,last_seen_days,contact,order,limit,"
           + "offset,fields,family").split(",")));
 
   private static Set<String> illegalSearchQualifiers =
-      new HashSet<String>(Arrays.asList(("search,fingerprint,order,limit,"
+      new HashSet<>(Arrays.asList(("search,fingerprint,order,limit,"
           + "offset,fields").split(",")));
 
   /** Handles the HTTP GET request in the wrapped <code>request</code> by
@@ -124,7 +125,7 @@ public class ResourceServlet extends HttpServlet {
 
     /* Extract parameters either from the old-style URI or from request
      * parameters. */
-    Map<String, String> parameterMap = new HashMap<String, String>();
+    Map<String, String> parameterMap = new HashMap<>();
     for (Object parameterKey : request.getParameterMap().keySet()) {
       String[] parameterValues =
           request.getParameterValues((String) parameterKey);
@@ -148,7 +149,7 @@ public class ResourceServlet extends HttpServlet {
         response.sendError(HttpServletResponse.SC_BAD_REQUEST);
         return;
       }
-      List<String> unqualifiedSearchTerms = new ArrayList<String>();
+      List<String> unqualifiedSearchTerms = new ArrayList<>();
       for (String searchTerm : searchTerms) {
         if (searchTerm.contains(":") && !searchTerm.startsWith("[")) {
           String[] parts = searchTerm.split(":", 2);
@@ -483,8 +484,9 @@ public class ResourceServlet extends HttpServlet {
       Pattern.compile("^[0-9a-zA-Z_,-]*$");
 
   private static HashSet<String> knownOrderParameters = new HashSet<>(
-      Arrays.asList(new String[] { "consensus_weight", "-consensus_weight",
-          "first_seen", "-first_seen" }));
+      Arrays.asList(new String[] { Fields.CONSENSUS_WEIGHT_ASC,
+          Fields.CONSENSUS_WEIGHT_DES, Fields.FIRST_SEEN_ASC,
+          Fields.FIRST_SEEN_DES }));
 
   private String[] parseOrderParameter(String parameter) {
     if (!orderParameterPattern.matcher(parameter).matches()) {
diff --git a/src/main/java/org/torproject/onionoo/server/ResponseBuilder.java b/src/main/java/org/torproject/onionoo/server/ResponseBuilder.java
index 37ca1e4..c3fca8b 100644
--- a/src/main/java/org/torproject/onionoo/server/ResponseBuilder.java
+++ b/src/main/java/org/torproject/onionoo/server/ResponseBuilder.java
@@ -1,4 +1,4 @@
-/* Copyright 2011--2016 The Tor Project
+/* Copyright 2011--2017 The Tor Project
  * See LICENSE for licensing information */
 
 package org.torproject.onionoo.server;
@@ -8,6 +8,7 @@ import org.torproject.onionoo.docs.ClientsDocument;
 import org.torproject.onionoo.docs.DetailsDocument;
 import org.torproject.onionoo.docs.DocumentStore;
 import org.torproject.onionoo.docs.DocumentStoreFactory;
+import org.torproject.onionoo.docs.Fields;
 import org.torproject.onionoo.docs.SummaryDocument;
 import org.torproject.onionoo.docs.UptimeDocument;
 import org.torproject.onionoo.docs.WeightsDocument;
@@ -47,15 +48,13 @@ public class ResponseBuilder {
     this.bridgesPublishedString = bridgesPublishedString;
   }
 
-  private List<SummaryDocument> orderedRelays =
-      new ArrayList<SummaryDocument>();
+  private List<SummaryDocument> orderedRelays = new ArrayList<>();
 
   public void setOrderedRelays(List<SummaryDocument> orderedRelays) {
     this.orderedRelays = orderedRelays;
   }
 
-  private List<SummaryDocument> orderedBridges =
-      new ArrayList<SummaryDocument>();
+  private List<SummaryDocument> orderedBridges = new ArrayList<>();
 
   public void setOrderedBridges(List<SummaryDocument> orderedBridges) {
     this.orderedBridges = orderedBridges;
@@ -207,7 +206,7 @@ public class ResponseBuilder {
           } else if (field.equals("last_changed_address_or_port")) {
             dd.setLastChangedAddressOrPort(
                 detailsDocument.getLastChangedAddressOrPort());
-          } else if (field.equals("first_seen")) {
+          } else if (field.equals(Fields.FIRST_SEEN)) {
             dd.setFirstSeen(detailsDocument.getFirstSeen());
           } else if (field.equals("running")) {
             dd.setRunning(detailsDocument.getRunning());
@@ -229,7 +228,7 @@ public class ResponseBuilder {
             dd.setAsNumber(detailsDocument.getAsNumber());
           } else if (field.equals("as_name")) {
             dd.setAsName(detailsDocument.getAsName());
-          } else if (field.equals("consensus_weight")) {
+          } else if (field.equals(Fields.CONSENSUS_WEIGHT)) {
             dd.setConsensusWeight(detailsDocument.getConsensusWeight());
           } else if (field.equals("host_name")) {
             dd.setHostName(detailsDocument.getHostName());