[GitHub] [tinkerpop] nastra commented on a change in pull request #1120: gt/gte/lt/lte can throw CCE if object isn't a Comparable

[GitBox <gi...@apache.org>]

nastra commented on a change in pull request #1120: gt/gte/lt/lte can throw CCE if object isn't a Comparable
URL: https://github.com/apache/tinkerpop/pull/1120#discussion_r291518286
 
 

 ##########
 File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Compare.java
 ##########
 @@ -167,6 +181,20 @@ public Compare negate() {
         }
     };
 
+    private static boolean bothAreNumber(Object first, Object second) {
+        return first instanceof Number && second instanceof Number;
+    }
+
+    private static boolean bothAreComparable(Object first, Object second) {
+        return first instanceof Comparable && second instanceof Comparable;
+    }
+
+    private static void checkBothAreNumberOrComparable(Object first, Object second) {
+        if (null != first && null != second && !bothAreNumber(first, second) && !bothAreComparable(first, second)) {
+            throw new IllegalArgumentException(String.format("'%s' and '%s' need to be an instance of Number or Comparable", first, second));
 
 Review comment:
   addressed both of your comments and also added a check to make sure that both are actually of the same type (with tests)

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services