Introduce {Max,Min}Of{Array,Collection} Refaster rules

[Stephan202]

Suggested commit message:

Introduce `{Max,Min}Of{Array,Collection}` Refaster rules (#1276)

While there, generalize the `{Max,Min}OfVarargs` rules.

[Stephan202]

"collection" here is not a copy-paste error: "collection" != Collection.

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[mohamedsamehsalah]

Clean 🧹

One small enquiry 💭

[mohamedsamehsalah]

Looking at the unit test and usage of Comparator#naturalOrder, it made me wonder if we can re-write the case when the natural order is used as a comparator to the Collections#max(collection) method.

===================================================================
diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java
--- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java	(revision 49825759b2310bc0ea8fd72d8e4842f9c5460313)
+++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java	(date 1723313983688)
@@ -138,8 +138,12 @@
     return Collections.max(Arrays.asList(new String[0]), naturalOrder());
   }
 
-  String testMaxOfCollection() {
-    return Collections.max(ImmutableSet.of("foo", "bar"), naturalOrder());
+  String testMaxOfCollectionComparator() {
+    return Collections.max(ImmutableSet.of("foo", "bar"), reverseOrder());
+  }
+
+  String testMaxOfCollectionNaturalOrder() {
+    return Collections.max(ImmutableSet.of("foo", "bar"));
   }
 
   int testMaxOfVarargs() {
Index: error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>ISO-8859-1
===================================================================
diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java
--- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java	(revision 49825759b2310bc0ea8fd72d8e4842f9c5460313)
+++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java	(date 1723313983685)
@@ -147,7 +147,11 @@
     return Arrays.stream(new String[0]).max(naturalOrder()).orElseThrow();
   }
 
-  String testMaxOfCollection() {
+  String testMaxOfCollectionComparator() {
+    return ImmutableSet.of("foo", "bar").stream().max(reverseOrder()).orElseThrow();
+  }
+
+  String testMaxOfCollectionNaturalOrder() {
     return ImmutableSet.of("foo", "bar").stream().max(naturalOrder()).orElseThrow();
   }
 
Index: error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java	(revision 49825759b2310bc0ea8fd72d8e4842f9c5460313)
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java	(date 1723313988286)
@@ -363,7 +363,7 @@
    * Avoid unnecessary creation of a {@link Stream} to determine the maximum of a known collection
    * of values.
    */
-  static final class MaxOfCollection<S, T extends S> {
+  static final class MaxOfCollectionComparator<S, T extends S> {
     @BeforeTemplate
     T before(Collection<T> collection, Comparator<S> cmp) {
       return collection.stream().max(cmp).orElseThrow();
@@ -375,6 +375,22 @@
     }
   }
 
+  /**
+   * Avoid unnecessary creation of a {@link Stream} to determine the maximum of a known collection
+   * of values.
+   */
+  static final class MaxOfCollectionNaturalOrder<T extends Comparable<? super T>> {
+    @BeforeTemplate
+    T before(Collection<T> collection) {
+      return collection.stream().max(naturalOrder()).orElseThrow();
+    }
+
+    @AfterTemplate
+    T after(Collection<T> collection) {
+      return Collections.max(collection);
+    }
+  }
+
   /**
    * Avoid unnecessary creation of a {@link Stream} to determine the maximum of a known collection
    * of values.

[mohamedsamehsalah]

Or perhaps

Collections.max(collection, naturalOrder());

to

Collections.max(collection);

could be its own rule 👀

[Stephan202]

Indeed, it should be a separate rule, for maximum applicability. And likewise for min, of course, and with naturalOrder() and reverseOrder() variants. Are you up for a PR? 😄

[mohamedsamehsalah]

Makes sense 👌

Are you up for a PR? 😄

Always 🫡

[mohamedsamehsalah]

#1297

[mohamedsamehsalah]

🚀

[mohamedsamehsalah]

Makes sense 👌

Are you up for a PR? 😄

Always 🫡

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud