code quality improvement

caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/node/AddToString.java

@@ -33,7 +33,7 @@ protected boolean applies() {
 
   @Override
   protected void execute() {
-    String statement = "return String.format(\"%s=[key=%s, value=%s, weight=%d, moveCount=%,d, "
+    String statement = "return String.format(\"%s=[key=%s, value=%s, weight=%d, queueType=%,d, "
         + "accessTimeNS=%,d, \"\n+ \"writeTimeNS=%,d, prevInAccess=%s, nextInAccess=%s, "
         + "prevInWrite=%s, nextInWrite=%s]\",\ngetClass().getSimpleName(), getKey(), getValue(), "
         + "getWeight(), getQueueType(), \ngetAccessTime(), getWriteTime(), "

caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java

@@ -582,34 +582,31 @@ void expireEntries() {
     expireAfterWriteEntries(now);
   }
 
-  /** Expires entries on the access-order queue. */
+  /** Expires entries in the access-order queue. */
   @GuardedBy("evictionLock")
   void expireAfterAccessEntries(long now) {
-    if (expiresAfterAccess()) {
-      long expirationTime = now - expiresAfterAccessNanos();
-      for (;;) {
-        final Node<K, V> node = accessOrderEdenDeque().peekFirst();
-        if ((node == null) || (node.getAccessTime() > expirationTime)) {
-          break;
-        }
-        evictEntry(node, RemovalCause.EXPIRED, now);
-      }
-      if (evicts()) {
-        for (;;) {
-          final Node<K, V> node = accessOrderProbationDeque().peekFirst();
-          if ((node == null) || (node.getAccessTime() > expirationTime)) {
-            break;
-          }
-          evictEntry(node, RemovalCause.EXPIRED, now);
-        }
-        for (;;) {
-          final Node<K, V> node = accessOrderProtectedDeque().peekFirst();
-          if ((node == null) || (node.getAccessTime() > expirationTime)) {
-            break;
-          }
-          evictEntry(node, RemovalCause.EXPIRED, now);
-        }
+    if (!expiresAfterAccess()) {
+      return;
+    }
+
+    long expirationTime = (now - expiresAfterAccessNanos());
+    expireAfterAccessEntries(accessOrderEdenDeque(), expirationTime, now);
+    if (evicts()) {
+      expireAfterAccessEntries(accessOrderProbationDeque(), expirationTime, now);
+      expireAfterAccessEntries(accessOrderProtectedDeque(), expirationTime, now);
+    }
+  }
+
+  /** Expires entries in an access-order queue. */
+  @GuardedBy("evictionLock")
+  void expireAfterAccessEntries(AccessOrderDeque<Node<K, V>> accessOrderDeque,
+      long expirationTime, long now) {
+    for (;;) {
+      final Node<K, V> node = accessOrderDeque.peekFirst();
+      if ((node == null) || (node.getAccessTime() > expirationTime)) {
+        break;
       }
+      evictEntry(node, RemovalCause.EXPIRED, now);
     }
   }
 
@@ -693,10 +690,10 @@ void evictEntry(Node<K, V> node, RemovalCause cause, long now) {
     // the addition that triggered this eviction. The victim is eagerly unlinked before the removal
     // task so that if an eviction is still required then a new victim will be chosen for removal.
     makeDead(node);
-    if (node.isEden() && (evicts() || expiresAfterAccess())) {
+    if (node.inEden() && (evicts() || expiresAfterAccess())) {
       accessOrderEdenDeque().remove(node);
     } else if (evicts()) {
-      if (node.isMainProbation()) {
+      if (node.inMainProbation()) {
         accessOrderProbationDeque().remove(node);
       } else {
         accessOrderProtectedDeque().remove(node);
@@ -916,33 +913,10 @@ void onAccess(Node<K, V> node) {
         return;
       }
       frequencySketch().increment(key);
-      if (node.isEden()) {
+      if (node.inEden()) {
         reorder(accessOrderEdenDeque(), node);
-      } else if (node.isMainProbation()) {
-        if (!accessOrderProbationDeque().contains(node)) {
-          // Ignore stale accesses for an entry that is no longer present
-          return;
-        } else if (node.getPolicyWeight() > mainProtectedMaximum()) {
-          return;
-        }
-
-        long mainProtectedWeightedSize = mainProtectedWeightedSize() + node.getPolicyWeight();
-        accessOrderProbationDeque().remove(node);
-        accessOrderProtectedDeque().add(node);
-        node.makeMainProtected();
-
-        long mainProtectedMaximum = mainProtectedMaximum();
-        while (mainProtectedWeightedSize > mainProtectedMaximum) {
-          Node<K, V> demoted = accessOrderProtectedDeque().pollFirst();
-          if (demoted == null) {
-            break;
-          }
-          demoted.makeMainProbation();
-          accessOrderProbationDeque().add(demoted);
-          mainProtectedWeightedSize -= node.getPolicyWeight();
-        }
-
-        lazySetMainProtectedWeightedSize(mainProtectedWeightedSize);
+      } else if (node.inMainProbation()) {
+        reorderProbation(node);
       } else {
         reorder(accessOrderProtectedDeque(), node);
       }
@@ -951,6 +925,35 @@ void onAccess(Node<K, V> node) {
     }
   }
 
+  /** Updates the node's location in the probation queue. */
+  @GuardedBy("evictionLock")
+  void reorderProbation(Node<K, V> node) {
+    if (!accessOrderProbationDeque().contains(node)) {
+      // Ignore stale accesses for an entry that is no longer present
+      return;
+    } else if (node.getPolicyWeight() > mainProtectedMaximum()) {
+      return;
+    }
+
+    long mainProtectedWeightedSize = mainProtectedWeightedSize() + node.getPolicyWeight();
+    accessOrderProbationDeque().remove(node);
+    accessOrderProtectedDeque().add(node);
+    node.makeMainProtected();
+
+    long mainProtectedMaximum = mainProtectedMaximum();
+    while (mainProtectedWeightedSize > mainProtectedMaximum) {
+      Node<K, V> demoted = accessOrderProtectedDeque().pollFirst();
+      if (demoted == null) {
+        break;
+      }
+      demoted.makeMainProbation();
+      accessOrderProbationDeque().add(demoted);
+      mainProtectedWeightedSize -= node.getPolicyWeight();
+    }
+
+    lazySetMainProtectedWeightedSize(mainProtectedWeightedSize);
+  }
+
   /** Updates the node's location in the policy's deque. */
   static <K, V> void reorder(LinkedDeque<Node<K, V>> deque, Node<K, V> node) {
     // An entry may be scheduled for reordering despite having been removed. This can occur when the
@@ -989,9 +992,9 @@ void makeDead(Node<K, V> node) {
         // The node's policy weight may be out of sync due to a pending update waiting to be
         // processed. At this point the node's weight is finalized, so the weight can be safely
         // taken from the node's perspective and the sizes will be adjusted correctly.
-        if (node.isEden()) {
+        if (node.inEden()) {
           lazySetEdenWeightedSize(edenWeightedSize() - node.getWeight());
-        } else if (node.isMainProtected()) {
+        } else if (node.inMainProtected()) {
           lazySetMainProtectedWeightedSize(mainProtectedWeightedSize() - node.getWeight());
         }
         lazySetWeightedSize(weightedSize() - node.getWeight());
@@ -1074,10 +1077,10 @@ final class RemovalTask implements Runnable {
     @GuardedBy("evictionLock")
     public void run() {
       // add may not have been processed yet
-      if (node.isEden() && (evicts() || expiresAfterAccess())) {
+      if (node.inEden() && (evicts() || expiresAfterAccess())) {
         accessOrderEdenDeque().remove(node);
       } else if (evicts()) {
-        if (node.isMainProbation()) {
+        if (node.inMainProbation()) {
           accessOrderProbationDeque().remove(node);
         } else {
           accessOrderProtectedDeque().remove(node);
@@ -1104,9 +1107,9 @@ public UpdateTask(Node<K, V> node, int weightDifference) {
     @GuardedBy("evictionLock")
     public void run() {
       if (evicts()) {
-        if (node.isEden()) {
+        if (node.inEden()) {
           lazySetEdenWeightedSize(edenWeightedSize() + weightDifference);
-        } else if (node.isMainProtected()) {
+        } else if (node.inMainProtected()) {
           lazySetMainProtectedWeightedSize(mainProtectedMaximum() + weightDifference);
         }
         lazySetWeightedSize(weightedSize() + weightDifference);
@@ -2156,7 +2159,7 @@ public K next() {
 
     @Override
     public void remove() {
-      Caffeine.requireState(current != null);
+      requireState(current != null);
       BoundedLocalCache.this.remove(current);
       current = null;
     }
@@ -2292,7 +2295,7 @@ public Entry<K, V> next() {
 
     @Override
     public void remove() {
-      Caffeine.requireState(removalKey != null);
+      requireState(removalKey != null);
       BoundedLocalCache.this.remove(removalKey);
       removalKey = null;
     }
@@ -2527,11 +2530,13 @@ final class BoundedRefreshAfterWrite implements Expiration<K, V> {
         cache.setRefreshAfterWriteNanos(unit.toNanos(duration));
         cache.executor().execute(cache.drainBuffersTask);
       }
+      @SuppressWarnings("PMD.SimplifiedTernary") // false positive (#1424)
       @Override public Map<K, V> oldest(int limit) {
         return cache.expiresAfterWrite()
             ? expireAfterWrite().get().oldest(limit)
             : sortedByWriteTime(limit, true);
       }
+      @SuppressWarnings("PMD.SimplifiedTernary") // false positive (#1424)
       @Override public Map<K, V> youngest(int limit) {
         return cache.expiresAfterWrite()
             ? expireAfterWrite().get().youngest(limit)

caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncLoadingCache.java

@@ -548,8 +548,7 @@ public boolean replace(K key, V oldValue, V newValue) {
       requireNonNull(newValue);
       CompletableFuture<V> oldValueFuture = delegate.get(key);
       return oldValue.equals(Async.getIfReady(oldValueFuture))
-          ? delegate.replace(key, oldValueFuture, CompletableFuture.completedFuture(newValue))
-          : false;
+          && delegate.replace(key, oldValueFuture, CompletableFuture.completedFuture(newValue));
     }
 
     @Override
@@ -560,8 +559,7 @@ public boolean remove(Object key, Object value) {
       }
       CompletableFuture<V> oldValueFuture = delegate.get(key);
       return value.equals(Async.getIfReady(oldValueFuture))
-          ? delegate.remove(key, oldValueFuture)
-          : false;
+          && delegate.remove(key, oldValueFuture);
     }
 
     @Override

caffeine/src/main/java/com/github/benmanes/caffeine/cache/Node.java

@@ -125,17 +125,17 @@ default void setPolicyWeight(int weight) {}
   int PROTECTED = 2;
 
   /** Returns if the entry is in the Eden or Main space. */
-  default boolean isEden() {
+  default boolean inEden() {
     return getQueueType() == EDEN;
   }
 
   /** Returns if the entry is in the Main space's probation queue. */
-  default boolean isMainProbation() {
+  default boolean inMainProbation() {
     return getQueueType() == PROBATION;
   }
 
   /** Returns if the entry is in the Main space's protected queue. */
-  default boolean isMainProtected() {
+  default boolean inMainProtected() {
     return getQueueType() == PROTECTED;
   }
 

caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheValidationListener.java

@@ -33,6 +33,7 @@
 import java.util.Map;
 import java.util.Objects;
 
+import org.apache.commons.lang3.StringUtils;
 import org.testng.IInvokedMethod;
 import org.testng.IInvokedMethodListener;
 import org.testng.ITestResult;
@@ -73,12 +74,18 @@ public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
       }
     } catch (Throwable caught) {
       testResult.setStatus(ITestResult.FAILURE);
-      testResult.setThrowable(caught);
+      testResult.setThrowable(new AssertionError(getTestName(method), caught));
     } finally {
       cleanUp(testResult);
     }
   }
 
+  /** Returns the name of the executed test. */
+  private static String getTestName(IInvokedMethod method) {
+    return StringUtils.substringAfterLast(method.getTestMethod().getTestClass().getName(), ".")
+        + "#" + method.getTestMethod().getConstructorOrMethod().getName();
+  }
+
   /** Checks whether the {@link TrackingExecutor} had unexpected failures. */
   private static void checkExecutor(ITestResult testResult, CacheContext context) {
     Method testMethod = testResult.getMethod().getConstructorOrMethod().getMethod();

config/pmd/rulesSets.xml

@@ -6,7 +6,6 @@
 
   <rule ref="rulesets/java/basic.xml">
     <exclude name="AvoidBranchingStatementAsLastInLoop"/>
-    <exclude name="UselessParentheses"/>
   </rule>
 
   <rule ref="rulesets/java/braces.xml"/>
@@ -28,6 +27,7 @@
   <rule ref="rulesets/java/comments.xml">
     <exclude name="CommentSize"/>
     <exclude name="CommentRequired"/>
+    <exclude name="CommentDefaultAccessModifier"/>
   </rule>
 
   <rule ref="rulesets/java/coupling.xml">

gradle/code_quality.gradle

@@ -35,7 +35,7 @@ findbugs {
 
 pmd {
   ruleSets = []
-  toolVersion = '5.3.3'
+  toolVersion = '5.4.1'
   ruleSetConfig = resources.text.fromFile(file("${rootDir}/config/pmd/rulesSets.xml"))
 }
 

jcache/src/main/java/com/github/benmanes/caffeine/jcache/CacheFactory.java

@@ -116,7 +116,6 @@ private final class Builder<K, V> {
     CacheLoader<K, V> cacheLoader;
     JCacheEvictionListener<K, V> evictionListener;
 
-    @SuppressWarnings("PMD.StringToString")
     Builder(String cacheName, CaffeineConfiguration<K, V> config) {
       this.config = config;
       this.cacheName = cacheName;

jcache/src/main/java/com/github/benmanes/caffeine/jcache/configuration/CaffeineConfiguration.java

@@ -38,8 +38,8 @@
  * A JCache configuration with Caffeine specific settings.
  * <p>
  * The initial settings disable <tt>store by value</tt> so that entries are not copied when crossing
- * the {@link javax.cache.Cache} API boundary. If enabled and the {@link Copier} is explicitly set,
- * then the {@link JavaSerializationCopier} will be used. This differs from
+ * the {@link javax.cache.Cache} API boundary. If enabled and the {@link Copier} is not explicitly
+ * set, then the {@link JavaSerializationCopier} will be used. This differs from
  * {@link MutableConfiguration} which enables <tt>store by value</tt> at construction.
  *
  * @author [email protected] (Ben Manes)

jcache/src/main/java/com/github/benmanes/caffeine/jcache/configuration/TypesafeConfigurator.java

@@ -43,6 +43,7 @@
  *
  * @author [email protected] (Ben Manes)
  */
+@SuppressWarnings("PMD.AvoidDuplicateLiterals")
 public final class TypesafeConfigurator {
   static final Logger logger = Logger.getLogger(TypesafeConfigurator.class.getName());