From a480d1e656ef85809ce86acb4a30db5d3285d874 Mon Sep 17 00:00:00 2001 From: lhy-hoyin <88828097+lhy-hoyin@users.noreply.github.com> Date: Tue, 29 Oct 2024 11:17:44 +0800 Subject: [PATCH] Integrate replacement logic directly into add to reduce code complexity. All tests passed. --- .../impl/lang/DefaultCollectionMutator.java | 67 +++++++------------ .../lang/DefaultCollectionMutatorTest.groovy | 62 ++++++----------- 2 files changed, 47 insertions(+), 82 deletions(-) diff --git a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java index 5130e3160..f2f1286f8 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java @@ -16,7 +16,6 @@ package io.jsonwebtoken.impl.lang; import io.jsonwebtoken.Identifiable; -import io.jsonwebtoken.lang.Assert; import io.jsonwebtoken.lang.CollectionMutator; import io.jsonwebtoken.lang.Collections; import io.jsonwebtoken.lang.Objects; @@ -25,7 +24,6 @@ import java.util.Collection; import java.util.Iterator; import java.util.LinkedHashSet; -import java.util.NoSuchElementException; public class DefaultCollectionMutator> implements CollectionMutator { @@ -49,59 +47,46 @@ private boolean doAdd(E e) { return this.collection.add(e); } - public M replace(E existingElement, E newElement) { - Assert.notEmpty(existingElement, "existingElement cannot be null or empty."); - Assert.notEmpty(newElement, "newElement cannot be null or empty."); - - // Same item, nothing to do - if (existingElement.equals(newElement)) - return self(); - - // Does not contain existingElement to replace - if (!this.collection.contains(existingElement)) { - String msg = this.getClass() + " does not contain " + existingElement + "."; - throw new NoSuchElementException(msg); - } + @Override + public M add(E e) { + boolean doReplace = false; - // Replacement step 1: iterate until element to replace + // Replacement step 1: iterate until element to replace (if any) + E item; Iterator it = this.collection.iterator(); - while (it.hasNext()) - if (it.next().equals(existingElement)) { - it.remove(); // step 2: remove existingElement - break; - } - - // Replacement step 3: collect and remove elements after element to replace - Collection elementsAfterExisting = new LinkedHashSet<>(); while (it.hasNext()) { - elementsAfterExisting.add(it.next()); - it.remove(); - } - - this.doAdd(newElement); // step 4: add replacer element (position will be at the existingElement) - this.collection.addAll(elementsAfterExisting); // step 5: add back the elemnts found after existingElement + item = it.next(); - changed(); // trigger changed() + // Same item, nothing to do + if (item.equals(e)) + return self(); - return self(); - } - - @Override - public M add(E e) { - E existing = null; - for (E item : collection) { boolean bothIdentifiable = e instanceof Identifiable && item instanceof Identifiable; boolean sameId = bothIdentifiable && ((Identifiable) item).getId().equals(((Identifiable) e).getId()); if (sameId) { - existing = item; + it.remove(); // step 2: remove existing item + doReplace = true; break; } } - if (Objects.isEmpty(existing)) { + if (doReplace) { + // Replacement step 3: collect and remove elements after element to replace + Collection elementsAfterExisting = new LinkedHashSet<>(); + while (it.hasNext()) { + elementsAfterExisting.add(it.next()); + it.remove(); + } + + this.doAdd(e); // step 4: add replacer element (position will be at the existing item) + this.collection.addAll(elementsAfterExisting); // step 5: add back the elements found after existing item + + changed(); // trigger changed() + } + else { + // No replacement, do add instead if (doAdd(e)) changed(); } - else replace(existing, e); return self(); } diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy index 356cd8c4f..ce5797d0e 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy @@ -52,9 +52,17 @@ class DefaultCollectionMutatorTest { assertTrue c.isEmpty() } + @Test + void addNull() { + m.add(null) + assertEquals 0, changeCount + assertTrue m.getCollection().isEmpty() // wasn't added + } + @Test void addEmpty() { m.add(Strings.EMPTY) + assertEquals 0, changeCount assertTrue m.getCollection().isEmpty() // wasn't added } @@ -116,6 +124,19 @@ class DefaultCollectionMutatorTest { assertEquals 'bar', ((IdentifiableObject) m.collection.toArray()[0]).obj } + @Test + void addIdentifiableWithSameIdMaintainsOrder() { + IdentifiableObject e1 = new IdentifiableObject('1', 'e1') + IdentifiableObject e2 = new IdentifiableObject('sameId', 'e2') + IdentifiableObject e3 = new IdentifiableObject('3', 'e3') + IdentifiableObject eB = new IdentifiableObject('sameId', 'eB') + + m.add([e1, e2, e3]) + m.add(eB) // replace e2 with eB + assertEquals 2, changeCount + assertArrayEquals(new Object[] {e1, eB, e3}, m.collection.toArray()) + } + @Test void addSecureDigestAlgorithmWithSameIdReplacesExisting() { Class c = Class.forName("io.jsonwebtoken.impl.security.DefaultMacAlgorithm") @@ -143,47 +164,6 @@ class DefaultCollectionMutatorTest { assertEquals 0, changeCount } - @Test - void replace() { - def e1 = new IdentifiableObject('sameId', 'e1') - def e2 = new IdentifiableObject('sameId', 'e2') - - m.add(e1) - m.replace(e1, e2) - assertEquals 2, changeCount // replace is count as one change - assertEquals 1, m.getCollection().size() // existing is removed as part of replacement - assertEquals 'e2', ((IdentifiableObject) m.getCollection().toArray()[0]).obj - } - - @Test - void replaceSameObject() { - m.add('hello') - m.replace('hello', 'hello') // replace with the same object, no change should be reflected - assertEquals 1, changeCount - } - - @Test(expected = NoSuchElementException) - void replaceMissing() { - m.replace('foo', 'bar') - } - - @Test(expected = IllegalArgumentException) - void replaceNull() { - m.replace('foo', null) - } - - @Test(expected = IllegalArgumentException) - void replaceEmpty() { - m.replace('foo', Strings.EMPTY) - } - - @Test - void replaceMaintainsOrder() { - m.add(['1', '2', '3']) - m.replace('2', 'B') - assertArrayEquals(new Object[] {'1', 'B', '3'}, m.collection.toArray()) - } - @Test void clear() { m.add('one').add('two').add(['three', 'four'])