[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16102385#comment-16102385
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user asfgit closed the pull request at:

https://github.com/apache/geode/pull/580


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16102286#comment-16102286
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user YehEmily commented on a diff in the pull request:

https://github.com/apache/geode/pull/580#discussion_r129695770
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,54 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @Test
   public void testSupportedOrderByForRR() throws Exception {
-
 String unsupportedQueries[] =
-{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID",
-
-};
+{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID"};
 Object r[][] = new Object[unsupportedQueries.length][2];
-QueryService qs;
-qs = CacheUtils.getQueryService();
 Position.resetCounter();
-// Create Regions
 
+// Create Regions
 Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class);
 
 for (int i = 0; i < 50; i++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 for (int i = 0; i < unsupportedQueries.length; i++) {
-  Query q = null;
-
+  Query q;
   CacheUtils.getLogger().info("Executing query: " + 
unsupportedQueries[i]);
   q = CacheUtils.getQueryService().newQuery(unsupportedQueries[i]);
   try {
 r[i][0] = q.execute();
-
   } catch (QueryInvalidException qe) {
 qe.printStackTrace();
 fail(qe.toString());
   }
 }
   }
 
+  // The following tests cover edge cases in OrderByComparator.
+  @Test
+  public void testCompareTwoNulls() throws Exception {
+assert (createComparator().compare(null, null) == 0);
--- End diff --

Thanks for catching this! I've fixed it and updated the PR.


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16102269#comment-16102269
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/geode/pull/580#discussion_r129693128
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,54 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @Test
   public void testSupportedOrderByForRR() throws Exception {
-
 String unsupportedQueries[] =
-{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID",
-
-};
+{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID"};
 Object r[][] = new Object[unsupportedQueries.length][2];
-QueryService qs;
-qs = CacheUtils.getQueryService();
 Position.resetCounter();
-// Create Regions
 
+// Create Regions
 Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class);
 
 for (int i = 0; i < 50; i++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 for (int i = 0; i < unsupportedQueries.length; i++) {
-  Query q = null;
-
+  Query q;
   CacheUtils.getLogger().info("Executing query: " + 
unsupportedQueries[i]);
   q = CacheUtils.getQueryService().newQuery(unsupportedQueries[i]);
   try {
 r[i][0] = q.execute();
-
   } catch (QueryInvalidException qe) {
 qe.printStackTrace();
 fail(qe.toString());
   }
 }
   }
 
+  // The following tests cover edge cases in OrderByComparator.
+  @Test
+  public void testCompareTwoNulls() throws Exception {
+assert (createComparator().compare(null, null) == 0);
+  }
+
+  @Test
+  public void testCompareTwoObjectArrays() throws Exception {
+String[] arrString1 = {"elephant"};
+String[] arrString2 = {"elephants"};
+assert (createComparator().compare(arrString2, arrString1) == 1);
--- End diff --

Same as previous comment. Please use assertThat.


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16102268#comment-16102268
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user kirklund commented on a diff in the pull request:

https://github.com/apache/geode/pull/580#discussion_r129693040
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,54 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @Test
   public void testSupportedOrderByForRR() throws Exception {
-
 String unsupportedQueries[] =
-{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID",
-
-};
+{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID"};
 Object r[][] = new Object[unsupportedQueries.length][2];
-QueryService qs;
-qs = CacheUtils.getQueryService();
 Position.resetCounter();
-// Create Regions
 
+// Create Regions
 Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class);
 
 for (int i = 0; i < 50; i++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 for (int i = 0; i < unsupportedQueries.length; i++) {
-  Query q = null;
-
+  Query q;
   CacheUtils.getLogger().info("Executing query: " + 
unsupportedQueries[i]);
   q = CacheUtils.getQueryService().newQuery(unsupportedQueries[i]);
   try {
 r[i][0] = q.execute();
-
   } catch (QueryInvalidException qe) {
 qe.printStackTrace();
 fail(qe.toString());
   }
 }
   }
 
+  // The following tests cover edge cases in OrderByComparator.
+  @Test
+  public void testCompareTwoNulls() throws Exception {
+assert (createComparator().compare(null, null) == 0);
--- End diff --

"assert" should be used in product code instead of tests. These are the 
optional assertions that are enabled when running a Java product with -ea 
(enable assertions).

You should use assertThat instead:
```java
assertThat(createComparator().compare(null, null).isEqualTo(0);
```


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16100589#comment-16100589
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user YehEmily commented on a diff in the pull request:

https://github.com/apache/geode/pull/580#discussion_r129399374
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,58 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @Test
   public void testSupportedOrderByForRR() throws Exception {
-
 String unsupportedQueries[] =
-{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID",
-
-};
+{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID"};
 Object r[][] = new Object[unsupportedQueries.length][2];
-QueryService qs;
-qs = CacheUtils.getQueryService();
 Position.resetCounter();
-// Create Regions
 
+// Create Regions
 Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class);
 
 for (int i = 0; i < 50; i++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 for (int i = 0; i < unsupportedQueries.length; i++) {
-  Query q = null;
-
+  Query q;
   CacheUtils.getLogger().info("Executing query: " + 
unsupportedQueries[i]);
   q = CacheUtils.getQueryService().newQuery(unsupportedQueries[i]);
   try {
 r[i][0] = q.execute();
-
   } catch (QueryInvalidException qe) {
 qe.printStackTrace();
 fail(qe.toString());
   }
 }
   }
 
+  /**
+   * Tests three cases that were not originally covered by the original 
tests in this class. (To
--- End diff --

All good points - I've fixed this and updated the PR!


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16100590#comment-16100590
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user YehEmily commented on a diff in the pull request:

https://github.com/apache/geode/pull/580#discussion_r129399451
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,58 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @Test
   public void testSupportedOrderByForRR() throws Exception {
-
 String unsupportedQueries[] =
-{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID",
-
-};
+{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID"};
 Object r[][] = new Object[unsupportedQueries.length][2];
-QueryService qs;
-qs = CacheUtils.getQueryService();
 Position.resetCounter();
-// Create Regions
 
+// Create Regions
 Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class);
 
 for (int i = 0; i < 50; i++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 for (int i = 0; i < unsupportedQueries.length; i++) {
-  Query q = null;
-
+  Query q;
   CacheUtils.getLogger().info("Executing query: " + 
unsupportedQueries[i]);
   q = CacheUtils.getQueryService().newQuery(unsupportedQueries[i]);
   try {
 r[i][0] = q.execute();
-
   } catch (QueryInvalidException qe) {
 qe.printStackTrace();
 fail(qe.toString());
   }
 }
   }
 
+  /**
+   * Tests three cases that were not originally covered by the original 
tests in this class. (To
+   * confirm that these tests cover all cases in OrderByComparator, run 
them with coverage.)
+   */
+  @Test
+  public void testCompareMethodEdgeCases() throws Exception {
--- End diff --

Thanks for the feedback! I've updated the PR!


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16100579#comment-16100579
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user pdxrunner commented on a diff in the pull request:

https://github.com/apache/geode/pull/580#discussion_r129397800
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,58 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @Test
   public void testSupportedOrderByForRR() throws Exception {
-
 String unsupportedQueries[] =
-{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID",
-
-};
+{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID"};
 Object r[][] = new Object[unsupportedQueries.length][2];
-QueryService qs;
-qs = CacheUtils.getQueryService();
 Position.resetCounter();
-// Create Regions
 
+// Create Regions
 Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class);
 
 for (int i = 0; i < 50; i++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 for (int i = 0; i < unsupportedQueries.length; i++) {
-  Query q = null;
-
+  Query q;
   CacheUtils.getLogger().info("Executing query: " + 
unsupportedQueries[i]);
   q = CacheUtils.getQueryService().newQuery(unsupportedQueries[i]);
   try {
 r[i][0] = q.execute();
-
   } catch (QueryInvalidException qe) {
 qe.printStackTrace();
 fail(qe.toString());
   }
 }
   }
 
+  /**
+   * Tests three cases that were not originally covered by the original 
tests in this class. (To
+   * confirm that these tests cover all cases in OrderByComparator, run 
them with coverage.)
+   */
+  @Test
+  public void testCompareMethodEdgeCases() throws Exception {
--- End diff --

To adhere to the guideline that each test should only check thing, I would 
break this up into separate tests.


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16100565#comment-16100565
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user pdxrunner commented on a diff in the pull request:

https://github.com/apache/geode/pull/580#discussion_r129395826
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,58 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @Test
   public void testSupportedOrderByForRR() throws Exception {
-
 String unsupportedQueries[] =
-{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID",
-
-};
+{"select distinct p.status from /portfolio1 p order by p.status, 
p.ID"};
 Object r[][] = new Object[unsupportedQueries.length][2];
-QueryService qs;
-qs = CacheUtils.getQueryService();
 Position.resetCounter();
-// Create Regions
 
+// Create Regions
 Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class);
 
 for (int i = 0; i < 50; i++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 for (int i = 0; i < unsupportedQueries.length; i++) {
-  Query q = null;
-
+  Query q;
   CacheUtils.getLogger().info("Executing query: " + 
unsupportedQueries[i]);
   q = CacheUtils.getQueryService().newQuery(unsupportedQueries[i]);
   try {
 r[i][0] = q.execute();
-
   } catch (QueryInvalidException qe) {
 qe.printStackTrace();
 fail(qe.toString());
   }
 }
   }
 
+  /**
+   * Tests three cases that were not originally covered by the original 
tests in this class. (To
--- End diff --

This comment probably doesn't need to be a javadoc, and the note about 
coverage isn't needed. I'd make this comment say something like, "The following 
tests cover edge cases in OrderByComparator".


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16096688#comment-16096688
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user YehEmily commented on the issue:

https://github.com/apache/geode/pull/580
  
@jhuynh1 Thanks for the suggestion! I updated the tests in 
`OrderByComparatorJUnitTest.java` to cover all the cases in 
`OrderByComparator.java` and updated the PR.


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16049663#comment-16049663
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user jaredjstewart commented on a diff in the pull request:

https://github.com/apache/geode/pull/580#discussion_r122066276
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java
 ---
@@ -228,4 +139,55 @@ void addEvaluatedSortCriteria(Object row, 
ExecutionContext context)
 // No op
   }
 
+  private int compareHelperMethod(Object obj1, Object obj2) {
+if (obj1 == null || obj2 == null) {
+  return compareIfOneOrMoreNull(obj1, obj2);
+} else if (obj1 == QueryService.UNDEFINED || obj2 == 
QueryService.UNDEFINED) {
+  return compareIfOneOrMoreQueryServiceUndefined(obj1, obj2);
+} else {
+  return compareTwoObjects(obj1, obj2);
+}
+  }
+
+  private int compareIfOneOrMoreNull(Object obj1, Object obj2) {
+if (obj1 == null) {
+  return obj2 == null ? 0 : -1;
+} else {
+  return 1;
+}
+  }
+
+  private int compareIfOneOrMoreQueryServiceUndefined(Object obj1, Object 
obj2) {
+if (obj1 == QueryService.UNDEFINED) {
+  return obj2 == QueryService.UNDEFINED ? 0 : -1;
+} else {
+  return 1;
+}
+  }
+
+  private int compareTwoObjects(Object obj1, Object obj2) {
+if (obj1 instanceof Number && obj2 instanceof Number) {
+  return compareTwoNumbers(obj1, obj2);
+} else {
+  return compareTwoStrings(obj1, obj2);
+}
+  }
+
+  private int compareTwoNumbers(Object obj1, Object obj2) {
--- End diff --

I see it was not introduced by your changes, but the subtraction on 177 is 
not a safe way to compare doubles as it can result in under/overflow.  For 
example, consider the result of `compareTwoNumbers( Double.MAX_VALUE, -1d)`.  
Here's a safer way to do it:

```
  private int compareTwoNumbers(Object obj1, Object obj2) {
Number number1 = (Number) obj1;
Number number2 = (Number) obj2 ;
return Double.compare(number1.doubleValue(), number2.doubleValue());
  }
```


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16049621#comment-16049621
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user YehEmily commented on a diff in the pull request:

https://github.com/apache/geode/pull/580#discussion_r122055572
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java
 ---
@@ -45,78 +44,50 @@ public OrderByComparator(List 
orderByAttrs, ObjectType ob
   }
 
   /**
-   * Yogesh : This methods evaluates sort criteria and returns a ArrayList 
of Object[] arrays of
-   * evaluated criteria
+   * This method evaluates sort criteria and returns an ArrayList of 
Object[] arrays of the
+   * evaluated criteria.
* 
-   * @param value
-   * @return Object[]
+   * @param value the criteria to be evaluated.
+   * @return an Object array of Object arrays of the evaluated criteria.
*/
   protected Object[] evaluateSortCriteria(Object value) {
-
 CompiledSortCriterion csc;
 Object[] array = null;
 if (orderByAttrs != null) {
   array = new Object[orderByAttrs.size()];
-  Iterator orderiter = orderByAttrs.iterator();
+  Iterator orderIter = orderByAttrs.iterator();
   int i = 0;
-  while (orderiter.hasNext()) {
-csc = (CompiledSortCriterion) orderiter.next();
-Object[] arr = new Object[2];
-arr[0] = csc.evaluate(value, context);
-arr[1] = Boolean.valueOf(csc.getCriterion());
+  while (orderIter.hasNext()) {
--- End diff --

Fixed, and updating the PR now - thank you!


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16049614#comment-16049614
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

Github user PurelyApplied commented on a diff in the pull request:

https://github.com/apache/geode/pull/580#discussion_r122053903
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java
 ---
@@ -45,78 +44,50 @@ public OrderByComparator(List 
orderByAttrs, ObjectType ob
   }
 
   /**
-   * Yogesh : This methods evaluates sort criteria and returns a ArrayList 
of Object[] arrays of
-   * evaluated criteria
+   * This method evaluates sort criteria and returns an ArrayList of 
Object[] arrays of the
+   * evaluated criteria.
* 
-   * @param value
-   * @return Object[]
+   * @param value the criteria to be evaluated.
+   * @return an Object array of Object arrays of the evaluated criteria.
*/
   protected Object[] evaluateSortCriteria(Object value) {
-
 CompiledSortCriterion csc;
 Object[] array = null;
 if (orderByAttrs != null) {
   array = new Object[orderByAttrs.size()];
-  Iterator orderiter = orderByAttrs.iterator();
+  Iterator orderIter = orderByAttrs.iterator();
   int i = 0;
-  while (orderiter.hasNext()) {
-csc = (CompiledSortCriterion) orderiter.next();
-Object[] arr = new Object[2];
-arr[0] = csc.evaluate(value, context);
-arr[1] = Boolean.valueOf(csc.getCriterion());
+  while (orderIter.hasNext()) {
--- End diff --

This loop and the one below could read better with a "for each" loop.
```
for (CompiledSortCriterion csc: orderByAttrs){
```

Good refactoring overall.
+1 with or without the loop edit.


> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (GEODE-2936) Refactor OrderByComparator's compare method to reduce redundant code

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/GEODE-2936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16049500#comment-16049500
 ] 

ASF GitHub Bot commented on GEODE-2936:
---

GitHub user YehEmily opened a pull request:

https://github.com/apache/geode/pull/580

GEODE-2936: Refactoring OrderByComparator

[View the JIRA ticket 
here.](https://issues.apache.org/jira/browse/GEODE-2936)

`OrderByComparator.java` contained a lot of redundant and confusing code, 
as well as somewhat misleading comments (for example, the `compare` method was 
supposed to throw a `ClassCastException` if the parameters given could not be 
compared using this comparator, but it didn't. This has been fixed). Code that 
is shared by both the `compare` and `evaluateSortCriteria` methods has been 
moved to helper methods to improve readability and reduce redundancy.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in 
the commit message?

- [ ] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?

- [ ] Is your initial contribution a single, squashed commit?

- [ ] Does `gradlew build` run cleanly?

- [ ] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and
submit an update to your PR as soon as possible. If you need help, please 
send an
email to d...@geode.apache.org.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/YehEmily/geode GEODE-2936-2

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/geode/pull/580.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #580


commit 3ff6d0a2b4f097947e337f5567b641aeadfb0e22
Author: Jinmei Liao 
Date:   2017-06-07T23:10:56Z

GEODE-2936: Refactoring OrderByComparator to reduce redundant code and 
improve readability.




> Refactor OrderByComparator's compare method to reduce redundant code
> 
>
> Key: GEODE-2936
> URL: https://issues.apache.org/jira/browse/GEODE-2936
> Project: Geode
>  Issue Type: Bug
>  Components: querying
>Reporter: nabarun
>Assignee: Emily Yeh
>
> Issue:
> OrderByComparator's compare method has a lot of redundant code.
> Solution:
> These code sections can be modified to have one method call



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)