Optimize ObjectToStringComparator.compare() method

garydgregory · garydgregory · commit 71c905200de7 · 2025-09-24T19:14:38.000-04:00
Pick up #1449 and complete the change
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
@@ -46,6 +46,7 @@ The <action> type attribute can be add,update,fix,remove.
   <body>
   <release version="3.19.1" date="YYYY-MM-DD" description="This is a feature and maintenance release. Java 8 or later is required.">
     <!-- FIX -->
+    <action                   type="fix" dev="ggregory" due-to="mayuming, Gary Gregory">Optimize ObjectToStringComparator.compare() method #1449.</action>
     <!-- ADD -->
     <!-- UPDATE -->
   </release>
diff --git a/src/conf/spotbugs-exclude-filter.xml b/src/conf/spotbugs-exclude-filter.xml
@@ -208,7 +208,7 @@
   <Match>
     <Class name="org.apache.commons.lang3.compare.ObjectToStringComparator" />
     <Method name="compare" />
-    <Bug pattern="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE" />
+    <Bug pattern="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE, ES_COMPARING_STRINGS_WITH_EQ" />
   </Match>
 
   <!-- Reason: requireNonNull is supposed to take a nullable parameter,
diff --git a/src/main/java/org/apache/commons/lang3/compare/ObjectToStringComparator.java b/src/main/java/org/apache/commons/lang3/compare/ObjectToStringComparator.java
@@ -53,7 +53,7 @@ public ObjectToStringComparator() {
 
     @Override
     public int compare(final Object o1, final Object o2) {
-        if (o1 == null && o2 == null) {
+        if (o1 == o2) {
             return 0;
         }
         if (o1 == null) {
@@ -65,7 +65,7 @@ public int compare(final Object o1, final Object o2) {
         final String string1 = o1.toString();
         final String string2 = o2.toString();
         // No guarantee that toString() returns a non-null value, despite what Spotbugs thinks.
-        if (string1 == null && string2 == null) {
+        if (string1 == string2) {
             return 0;
         }
         if (string1 == null) {
diff --git a/src/test/java/org/apache/commons/lang3/compare/ObjectToStringComparatorTest.java b/src/test/java/org/apache/commons/lang3/compare/ObjectToStringComparatorTest.java
@@ -19,6 +19,7 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
 
 import java.util.Arrays;
 import java.util.List;
@@ -46,14 +47,38 @@ public String toString() {
     }
 
     @Test
-    void testNull() {
-        final List<Thing> things = Arrays.asList(null, new Thing("y"), null);
+    void testNulls() {
+        final Thing thing = new Thing("y");
+        final List<Thing> things = Arrays.asList(null, thing, null);
         things.sort(ObjectToStringComparator.INSTANCE);
         assertEquals("y", things.get(0).string);
+        assertEquals(3, things.size());
+        assertSame(thing, things.get(0));
         assertNull(things.get(1));
         assertNull(things.get(2));
     }
 
+    @Test
+    void testNullLeft() {
+        final Thing thing = new Thing("y");
+        final List<Thing> things = Arrays.asList(null, thing);
+        things.sort(ObjectToStringComparator.INSTANCE);
+        assertEquals("y", things.get(0).string);
+        assertEquals(2, things.size());
+        assertSame(thing, things.get(0));
+        assertNull(things.get(1));
+    }
+
+    @Test
+    void testNullRight() {
+        final Thing thing = new Thing("y");
+        final List<Thing> things = Arrays.asList(thing, null);
+        things.sort(ObjectToStringComparator.INSTANCE);
+        assertEquals(2, things.size());
+        assertSame(thing, things.get(0));
+        assertNull(things.get(1));
+    }
+
     @Test
     void testNullToString() {
         final List<Thing> things = Arrays.asList(new Thing(null), new Thing("y"), new Thing(null));
