refactor: Fix or suppress codesmell recommendations

Author: dmaccormack

SuppressWarnings.md

@@ -0,0 +1,88 @@
+# JSON→URL: common uses of @SuppressWarnings
+
+There are a number of places in the code where a `@SuppressWarnings`
+annotation is used. Rather than repeating the `WONTFIX` or `FALSE POSITIVE`
+justification for common use cases all over the code, this file serves as a
+reference.
+
+## !API!
+Items marked with `!API!` would break the existing public API if the
+PMD/sonar recommendation was followed. They're good candidates for future
+improvement.
+
+## Complexity
+
+This is reported by PMD via
+[CyclomaticComplexity](https://pmd.github.io/pmd/pmd_rules_java_design.html#cyclomaticcomplexity)
+and
+[NPathComplexity](https://pmd.github.io/pmd/pmd_rules_java_design.html#npathcomplexity),
+and
+reported by sonarlint via
+[java:S3776](https://rules.sonarsource.com/c/RSPEC-3776).
+
+While
+[complexity](https://en.wikipedia.org/wiki/Cyclomatic_complexity)
+is a thing one generally wants to avoid, in these cases the method's content is
+really a single, logical thought. The tool's recommendation is to reduce the
+complexity of the method, which usually means splitting the code into multiple
+methods. However, splitting it into multiple methods, in this case, won't
+allow them to be independently tested (a primary motivation), and it will likely
+make it harder to follow the logic due to the additional indirection.
+
+```java
+@SuppressWarnings({
+    // See SuppressWarnings.md#complexity
+    "PMD.CyclomaticComplexity",
+    "PMD.NPathComplexity",
+    "java:S3776"
+})
+```
+
+## Reassigning Loop Variables
+
+Generally you want to avoid assigning to loop variables. However, sometimes it's
+necessary to make the code function properly, and alternative implementations
+are far less readable.
+
+```java
+@SuppressWarnings({
+    // See SuppressWarnings.md
+    "PMD.AvoidReassigningLoopVariables", "java:S127"
+})
+```
+## Shadowing an instance variable with a local variable
+
+It's often a bad idea to shadow an instance variable with a local variable.
+However, in some cases that's exactly what you want, when the local variable
+has the same semantic meaning as the instance variable, but it:
+
+* serves as a cache (improves performance)
+* needs to have some logic applied before it can be used
+
+```java
+@SuppressWarnings("java:S1117") // See SuppressWarnings.md
+```
+
+## Shadowing a class/interface name
+
+It's usually bad practice to shadow a class or interface name that you
+reference. However, the way the JSON→URL API is intended to be used, a
+user should never attempt to use both at the same time. Each derived class is
+an implementation of the parent class for its implementation target (e.g.
+json.org, JSR374, etc), and the user should only ever be using the derived
+class.
+
+```java
+@SuppressWarnings("java:S2176") // See SuppressWarnings.md
+```
+
+## Type parameter names
+
+Names should follow a naming convention, and generics are no exception.
+JSON→URL does follow a convention, it's just not the default, and I
+couldn't find a PMD parameter that allows me to change the regex.
+
+```java
+@SuppressWarnings({"PMD.GenericsNaming", "java:S119"}) // See SuppressWarnings.md
+```
+

module/jsonurl-core/src/main/java/org/jsonurl/JsonUrlOptionable.java

@@ -39,6 +39,7 @@ public interface JsonUrlOptionable {
     /**
      * Get the options from the given object, if possible.
      */
+    @SuppressWarnings("java:S1168") // See SuppressWarnings.md - !API!
     static Set<JsonUrlOption> getJsonUrlOptions(Object obj) {
         if (obj instanceof JsonUrlOptionable) {
             return ((JsonUrlOptionable)obj).options();

module/jsonurl-core/src/main/java/org/jsonurl/ParseException.java

@@ -110,11 +110,15 @@ public int getColumn() {
     /**
      * Exception type description.
      */
+    @SuppressWarnings(
+        // false positive; It's overridden by subclasses
+        "java:S3400")
     protected String typeDescription() {
         return "parse error";
     }
 
     @Override
+    @SuppressWarnings("java:S1117") // See SuppressWarnings.md
     public String toString() {
         StringBuilder buf = new StringBuilder(64);
 

module/jsonurl-core/src/main/java/org/jsonurl/stream/AbstractCharIterator.java

@@ -81,7 +81,7 @@ public abstract class AbstractCharIterator implements CharIterator {
      * exception is thrown.
      * @param name Resource {@link Resource#getName() name}
      */
-    public AbstractCharIterator(String name) {
+    protected AbstractCharIterator(String name) {
         this(name, JsonUrlLimits.DEFAULT_MAX_PARSE_CHARS);
     }
 
@@ -91,7 +91,7 @@ public AbstractCharIterator(String name) {
      * @param limit maximum number of parsed characters before a
      *     {@link org.jsonurl.LimitException LimitException} is thrown. 
      */
-    public AbstractCharIterator(String name, long limit) {
+    protected AbstractCharIterator(String name, long limit) {
         this.name = name;
         this.limit = limit;
     }

module/jsonurl-core/src/main/java/org/jsonurl/stream/AbstractEventIterator.java

@@ -87,7 +87,7 @@ public abstract class AbstractEventIterator
      * Construct a new AbstractIterator.
      * @param text input text
      */
-    public AbstractEventIterator(
+    protected AbstractEventIterator(
             CharIterator text,
             JsonUrlLimits limits,
             Set<JsonUrlOption> options) {
@@ -168,6 +168,7 @@ public static JsonUrlEvent getTrueFalseNull(CharSequence text) { // NOPMD
      *
      * @param type the type found during parse
      */
+    @SuppressWarnings("java:S1117") // See SuppressWarnings.md
     protected void checkResultType(ValueType type) {
         if (!doneTypeCheck) {
             doneTypeCheck = true;
@@ -191,6 +192,7 @@ protected void checkResultType(ValueType type) {
      *
      * @see #checkResultType(ValueType)
      */
+    @SuppressWarnings("java:S1117") // See SuppressWarnings.md
     protected void checkResultTypeIsComposite() {
         if (!doneTypeCheck) {
             doneTypeCheck = true;

module/jsonurl-core/src/main/java/org/jsonurl/stream/AbstractGrammar.java

@@ -110,11 +110,6 @@ private enum State {
      */
     private static final String INTERNAL_PARSE_ERROR = "internal parse error";
 
-    /*
-     * empty string.
-     *
-    private static final String EMPTY_STRING = "";*/
-
     /**
      * Parse state stack.
      */
@@ -135,7 +130,6 @@ private enum State {
      * Buffered "next" event value.
      */
     private JsonUrlEvent savedEventValue;
-    //private Deque<JsonUrlEvent> savedEventStack = new LinkedList<>();
 
     /**
      * Current parse/nesting depth.
@@ -153,7 +147,7 @@ private enum State {
      * @param limits a valid JsonUrlLimits or null
      * @param options valid JsonUrlOptions or null
      */
-    public AbstractGrammar(
+    protected AbstractGrammar(
             CharIterator text,
             JsonUrlLimits limits,
             Set<JsonUrlOption> options) {

module/jsonurl-core/src/main/java/org/jsonurl/stream/JsonUrlGrammarAQF.java

@@ -138,7 +138,12 @@ private void decodeBang(StringBuilder decodedText, boolean isFirst) {
     }
 
     @Override
-    @SuppressWarnings("PMD.CyclomaticComplexity")
+    @SuppressWarnings({
+        "PMD.CyclomaticComplexity",
+        
+        // false positive
+        "java:S135"
+    })
     protected boolean readAndBufferLiteral() {
         final StringBuilder decodedText = this.decodedTextBuffer;
         decodedText.setLength(0);

module/jsonurl-core/src/main/java/org/jsonurl/text/JsonTextAppendable.java

@@ -29,5 +29,8 @@
  * @author David MacCormack
  * @since 2020-11-01
  */
+@SuppressWarnings(
+    // false positive - it's used by subclasses
+    "java:S2326")
 public interface JsonTextAppendable<A,R> extends JsonTextBuilder<R>, Appendable {
 }

module/jsonurl-core/src/main/java/org/jsonurl/text/JsonUrlTextAppender.java

@@ -396,26 +396,20 @@ private static <T extends Appendable> void appendCodePoint(
     /**
      * Test if the given text matches one of: true, false, null.
      */
-    @SuppressWarnings("PMD.CyclomaticComplexity")
+    @SuppressWarnings({"java:S3776", "PMD.CyclomaticComplexity"})
     private static boolean isTrueFalseNull(CharSequence text, int start, int end) {
         switch (end - start) {
         case 4:
             switch (text.charAt(0)) {
             case 't':
-                if (text.charAt(1) == 'r'
-                        && text.charAt(2) == 'u'
-                        && text.charAt(3) == 'e') {
-                    return true;
-                }
-                return false;
+                return text.charAt(1) == 'r'
+                    && text.charAt(2) == 'u'
+                    && text.charAt(3) == 'e';
 
             case 'n':
-                if (text.charAt(1) == 'u'
-                        && text.charAt(2) == 'l'
-                        && text.charAt(3) == 'l') {
-                    return true;
-                }
-                return false;
+                return text.charAt(1) == 'u'
+                    && text.charAt(2) == 'l'
+                    && text.charAt(3) == 'l';
 
             default:
                 return false;
@@ -424,14 +418,11 @@ private static boolean isTrueFalseNull(CharSequence text, int start, int end) {
             // fall through
 
         case 5:
-            if (text.charAt(0) == 'f'
-                    && text.charAt(1) == 'a'
-                    && text.charAt(2) == 'l'
-                    && text.charAt(3) == 's'
-                    && text.charAt(4) == 'e') {
-                return true;
-            }
-            return false;
+            return text.charAt(0) == 'f'
+                && text.charAt(1) == 'a'
+                && text.charAt(2) == 'l'
+                && text.charAt(3) == 's'
+                && text.charAt(4) == 'e';
 
         default:
             return false;
@@ -467,7 +458,12 @@ private static boolean contains(
      * @param options a valid JsonUrlOptions or null
      * @return true if dest was modified
      */
-    @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.NPathComplexity"})
+    @SuppressWarnings({
+        // See SuppressWarnings.md#complexity
+        "PMD.CyclomaticComplexity",
+        "PMD.NPathComplexity",
+        "java:S3776"
+    })
     private static <T extends Appendable> boolean appendLiteral(
             T dest,
             CharSequence text,

module/jsonurl-core/src/main/java/org/jsonurl/text/NumberBuilder.java

@@ -468,12 +468,18 @@ public boolean parse(CharSequence text, Set<JsonUrlOption> options) {
      * @param stop an index
      * @return true if the CharSequence was successfully parsed
      */
-    public boolean parse(//NOPMD
+    @SuppressWarnings({
+        // See SuppressWarnings.md#complexity
+        "PMD.CyclomaticComplexity",
+        "PMD.NPathComplexity",
+        "java:S3776"
+    })
+    public boolean parse(
             CharSequence text,
             int start,
             int stop,
             Set<JsonUrlOption> options) {
-        int pos = this.start = start; //NOPMD
+        int pos = this.start = start;
 
         char c = text.charAt(start); //NOPMD
 
@@ -612,7 +618,17 @@ public static boolean isNumber(
      * @param options a valid set of options or {@code null}
      * @return true if the CharSequence is a JSON&#x2192;URL number
      */
-    public static boolean isNumber(//NOPMD
+    @SuppressWarnings({
+        // See SuppressWarnings.md#complexity
+        "PMD.CyclomaticComplexity",
+        "PMD.NPathComplexity",
+
+        //
+        // this is part of the public API, and I don't want to break it
+        //
+        "java:S1172"
+    })
+    public static boolean isNumber(
             CharSequence text,
             int start,
             int stop,
@@ -1104,9 +1120,15 @@ private static int parseInteger(
      * @param stop stop index
      * @return a long
      */
+    @SuppressWarnings({
+        "PMD.ShortVariable",
+
+        // doing so is semantically meaningful in this case
+        "PMD.AvoidReassigningParameters"
+    })
     private static long parseLong(
             CharSequence text,
-            int start, // NOPMD
+            int start,
             int stop,
             int defaultValue) {
 
@@ -1115,9 +1137,9 @@ private static long parseLong(
         }
 
         long ret = 0;
-        boolean isneg = false; // NOPMD
+        boolean isneg = false;
 
-        char c = text.charAt(start); // NOPMD
+        char c = text.charAt(start);
 
         switch (c) {
         case MINUS: