From e69fde9fc443d79d58f45ae25f89c1d87c76893b Mon Sep 17 00:00:00 2001 From: jdcove2 <59747923+jdcove2@users.noreply.github.com> Date: Mon, 23 Dec 2024 20:06:37 +0000 Subject: [PATCH] improve performance of ShortNameComparator. (#1027) --- .../emissary/util/ShortNameComparator.java | 54 ++++++-- .../util/ShortNameComparatorTest.java | 129 +++++++++--------- 2 files changed, 102 insertions(+), 81 deletions(-) diff --git a/src/main/java/emissary/util/ShortNameComparator.java b/src/main/java/emissary/util/ShortNameComparator.java index 3329966266..08c0e5bb3c 100755 --- a/src/main/java/emissary/util/ShortNameComparator.java +++ b/src/main/java/emissary/util/ShortNameComparator.java @@ -3,6 +3,8 @@ import emissary.core.Family; import emissary.core.IBaseDataObject; +import org.apache.commons.lang3.Validate; + import java.io.Serializable; import java.util.Comparator; @@ -17,36 +19,60 @@ public class ShortNameComparator implements Comparator, Seriali @Override public int compare(IBaseDataObject obj1, IBaseDataObject obj2) { + Validate.isTrue(obj1 != null, "Required: obj1 != null"); + Validate.isTrue(obj2 != null, "Required: obj2 != true"); + String s1 = obj1.shortName(); String s2 = obj2.shortName(); + int index1 = s1.indexOf(Family.SEP); + int index2 = s2.indexOf(Family.SEP); - String[] p1 = s1.split(Family.SEP); - String[] p2 = s2.split(Family.SEP); - - for (int i = 1; i < p1.length; i++) { - - if (i >= p2.length) { + // Loop through each level in obj1's shortName. + while (index1 != -1) { + // If obj1's shortName has more levels than obj2's shortName then obj1 > obj2. + if (index2 == -1) { return 1; } - if (p1[i].equals(p2[i])) { + // Get start character of level. + index1 += Family.SEP.length(); + index2 += Family.SEP.length(); + + // Get character length of level. + final int nextIndex1 = s1.indexOf(Family.SEP, index1); + final int nextIndex2 = s2.indexOf(Family.SEP, index2); + final int length1 = ((nextIndex1 < 0) ? s1.length() : nextIndex1) - index1; + final int length2 = ((nextIndex2 < 0) ? s2.length() : nextIndex2) - index2; + + // If the obj1's level and obj2's level are the same, then go to the next level. + if (length1 == length2 && s1.regionMatches(index1, s2, index2, length1)) { + index1 = nextIndex1; + index2 = nextIndex2; + continue; } + // Otherwise, try comparing the levels as integers. try { - int i1 = Integer.parseInt(p1[i]); - int i2 = Integer.parseInt(p2[i]); - return i1 - i2; + int int1 = Integer.parseInt(s1, index1, index1 + length1, 10); + int int2 = Integer.parseInt(s2, index2, index2 + length2, 10); + + return int1 - int2; } catch (NumberFormatException e) { - return p1[i].compareTo(p2[i]); - } + // Otherwise, compare the levels as strings. + final String substring1 = s1.substring(index1, index1 + length1); + final String substring2 = s2.substring(index2, index2 + length2); + return substring1.compareTo(substring2); + } } - if (p2.length > p1.length) { + // If obj1's shortName has fewer levels than obj2's shortName then obj1 < obj2. + if (index2 != -1) { return -1; } + + // Otherwise, obj1 and obj2 compare as equivalent return 0; } - } diff --git a/src/test/java/emissary/util/ShortNameComparatorTest.java b/src/test/java/emissary/util/ShortNameComparatorTest.java index 26bb9f4c74..2986bc23df 100644 --- a/src/test/java/emissary/util/ShortNameComparatorTest.java +++ b/src/test/java/emissary/util/ShortNameComparatorTest.java @@ -2,94 +2,89 @@ import emissary.core.BaseDataObject; import emissary.core.DataObjectFactory; -import emissary.core.Family; import emissary.core.IBaseDataObject; import emissary.test.core.junit5.UnitTest; import org.junit.jupiter.api.Test; -import java.util.ArrayList; import java.util.List; +import java.util.stream.Collectors; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.api.Assertions.assertIterableEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; class ShortNameComparatorTest extends UnitTest { - private final byte[] nobytes = new byte[0]; - private static final String b = "foo"; - private final String ba = b + Family.SEP; + // @formatter: off + private static final String ABC = "abc"; + private static final String DEF = "def"; + private static final String FOO_ATT_ABC = "foo-att-abc"; + private static final String FOO_ATT_DEF = "foo-att-def"; + private static final String FOO_ATT_1 = "foo-att-1"; + private static final String FOO_ATT_2 = "foo-att-2"; + private static final String FOO_ATT_1_ATT_1 = "foo-att-1-att-1"; + private static final String FOO_ATT_1_ATT_2 = "foo-att-1-att-2"; + private static final String FOO_ATT_10 = "foo-att-10"; + private static final String FOO_ATT_10_ATT_1 = "foo-att-10-att-1"; + // @formatter: on + private static final ShortNameComparator COMPARATOR = new ShortNameComparator(); @Test - void testOrdering() { - final List l = new ArrayList<>(); + void testParameters() { + final ShortNameComparator comparator = new ShortNameComparator(); + final IBaseDataObject ibdo = new BaseDataObject(); - fillList(l); - l.sort(new ShortNameComparator()); - checkList(l); + assertThrows(IllegalArgumentException.class, () -> comparator.compare(null, ibdo)); + assertThrows(IllegalArgumentException.class, () -> comparator.compare(ibdo, null)); } @Test - void testNonsenseNames() { - final List l = new ArrayList<>(); - l.add(DataObjectFactory.getInstance(null, "foo-att-def")); - l.add(DataObjectFactory.getInstance(null, "foo-att-abc")); - l.sort(new ShortNameComparator()); - assertEquals("foo-att-abc", l.get(0).shortName(), "Bad components sort in alpha order"); - } - - private void fillList(final List l) { - l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "1")); - l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "3")); - l.add(DataObjectFactory.getInstance(this.nobytes, b)); - l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "3" + Family.getSep(2))); - l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "3" + Family.getSep(1))); - l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "2")); - l.add(DataObjectFactory.getInstance(this.nobytes, this.ba + "3" + Family.getSep(1) + Family.getSep(1))); - } - - private void checkList(final List l) { - assertEquals(b, l.get(0).shortName(), "Ordering of sort"); - assertEquals(this.ba + "1", l.get(1).shortName(), "Ordering of sort"); - assertEquals(this.ba + "2", l.get(2).shortName(), "Ordering of sort"); - assertEquals(this.ba + "3", l.get(3).shortName(), "Ordering of sort"); - assertEquals(this.ba + "3" + Family.getSep(1), l.get(4).shortName(), "Ordering of sort"); - assertEquals(this.ba + "3" + Family.getSep(1) + Family.getSep(1), l.get(5).shortName(), "Ordering of sort"); - assertEquals(this.ba + "3" + Family.getSep(2), l.get(6).shortName(), "Ordering of sort"); + void testComparator() { + check(ABC, DEF, 0); + check(FOO_ATT_ABC, FOO_ATT_DEF, -3); + check(FOO_ATT_1, FOO_ATT_2, -1); + check(FOO_ATT_1, FOO_ATT_10, -9); + check(FOO_ATT_1, FOO_ATT_1_ATT_1, -1); } @Test - void testImplComparator() { - final List l = new ArrayList<>(); - - fillList(l); - l.sort(new ShortNameComparator()); - checkList(l); - } - - @Test - void testSubclassedComparator() { - final String defaultPayloadClass = DataObjectFactory.getImplementingClass(); - DataObjectFactory.setImplementingClass(MyDataObject.class.getName()); - try { - final List l = new ArrayList<>(); - - fillList(l); - l.sort(new ShortNameComparator()); - checkList(l); - } catch (RuntimeException ex) { - fail("Cannot operate Comparator in subclass", ex); - } finally { - DataObjectFactory.setImplementingClass(defaultPayloadClass); - } + void testListSorting() { + // start with an unsorted list + List unsorted = List.of( + FOO_ATT_1, + FOO_ATT_10, + FOO_ATT_2, + FOO_ATT_10_ATT_1, + FOO_ATT_1_ATT_2, + FOO_ATT_1_ATT_1); + + // map to a list of IBDO, sort using Comparator, map back to a list of Strings + List actual = unsorted + .stream() + .map(name -> DataObjectFactory.getInstance(new byte[0], name)) + .sorted(COMPARATOR) + .map(IBaseDataObject::shortName) + .collect(Collectors.toList()); + + // list with shortnames in expected sort order + List expected = List.of( + FOO_ATT_1, + FOO_ATT_1_ATT_1, + FOO_ATT_1_ATT_2, + FOO_ATT_2, + FOO_ATT_10, + FOO_ATT_10_ATT_1); + + // verify that the sorted output is correct + assertIterableEquals(expected, actual, "List wasn't sorted in the expected order"); } - // An extension of BaseDataObject for testing the - // lower bounded generics on the comparator - public static class MyDataObject extends BaseDataObject { - static final long serialVersionUID = 7872122417333007868L; + private static void check(final String name1, final String name2, final int forwardResult) { + final IBaseDataObject ibdo1 = new BaseDataObject(null, name1); + final IBaseDataObject ibdo2 = new BaseDataObject(null, name2); + final ShortNameComparator snc = new ShortNameComparator(); - public MyDataObject(final byte[] data, final String name) { - super(data, name); - } + assertEquals(forwardResult, snc.compare(ibdo1, ibdo2)); + assertEquals(-forwardResult, snc.compare(ibdo2, ibdo1)); } }