split out collection OCP issues into a separate category

mebigfatguy · Dec 10, 2024 · 6aedf19 · 6aedf19
1 parent c7ffcaf
commit 6aedf19
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 2 deletions.
diff --git a/etc/bugrank.txt b/etc/bugrank.txt
@@ -173,6 +173,7 @@
 +0 BugPattern NRTL_NON_RECYCLEABLE_TAG_LIB
 +0 BugPattern NSE_NON_SYMMETRIC_EQUALS
 +0 BugPattern OCP_OVERLY_CONCRETE_PARAMETER
++0 BugPattern OCP_OVERLY_CONCRETE_COLLECTION_PARAMETER
 +0 BugPattern OC_OVERZEALOUS_CASTING
 +0 BugPattern ODN_ORPHANED_DOM_NODE
 +0 BugPattern OI_OPTIONAL_ISSUES_CHECKING_REFERENCE

diff --git a/etc/findbugs.xml b/etc/findbugs.xml
@@ -41,7 +41,7 @@
 
 	<Detector class="com.mebigfatguy.fbcontrib.detect.CyclomaticComplexity" speed="slow" reports="CC_CYCLOMATIC_COMPLEXITY" />
 
-	<Detector class="com.mebigfatguy.fbcontrib.detect.OverlyConcreteParameter" speed="slow" reports="OCP_OVERLY_CONCRETE_PARAMETER" />
+	<Detector class="com.mebigfatguy.fbcontrib.detect.OverlyConcreteParameter" speed="slow" reports="OCP_OVERLY_CONCRETE_PARAMETER,OCP_OVERLY_CONCRETE_COLLECTION_PARAMETER" />
 
 	<Detector class="com.mebigfatguy.fbcontrib.detect.ListIndexedIterating" speed="moderate" reports="LII_LIST_INDEXED_ITERATING" />
 
@@ -349,6 +349,7 @@
 	<BugPattern abbrev="SCI" type="SCI_SYNCHRONIZED_COLLECTION_ITERATORS" category="CORRECTNESS" />
 	<BugPattern abbrev="CC" type="CC_CYCLOMATIC_COMPLEXITY" category="STYLE" />
 	<BugPattern abbrev="OCP" type="OCP_OVERLY_CONCRETE_PARAMETER" category="STYLE" />
+	<BugPattern abbrev="OCP" type="OCP_OVERLY_CONCRETE_COLLECTION_PARAMETER" category="STYLE" />
 	<BugPattern abbrev="LII" type="LII_LIST_INDEXED_ITERATING" category="STYLE" />
 	<BugPattern abbrev="UCC" type="UCC_UNRELATED_COLLECTION_CONTENTS" category="STYLE" />
 	<BugPattern abbrev="DRE" type="DRE_DECLARED_RUNTIME_EXCEPTION" category="STYLE" />

diff --git a/etc/messages.xml b/etc/messages.xml
@@ -1951,6 +1951,41 @@ private void appendToList(List&lt;String&gt; list) {
 			]]>
 		</Details>
 	</BugPattern>
+
+	   <BugPattern type="OCP_OVERLY_CONCRETE_COLLECTION_PARAMETER">
+        <ShortDescription>Method needlessly defines Collection parameter with concrete classes</ShortDescription>
+        <LongDescription>{1}: {3}</LongDescription>
+        <Details>
+            <![CDATA[
+            <p>This method uses specific collection interface/classes for parameters when only methods defined in the
+            Collection class are used. Consider increasing the abstraction of the interface to
+            make low impact changes easier to accomplish in the future.</p>
+
+            <p>Take the following example:<br/>
+<pre><code>
+private void printList(List&lt;String&gt; list) {
+    for (String s : list) {
+        System.out.println(s);
+    }
+}
+</code></pre>
+
+                The parameter list is currently defined as an <code>List</code>, which is a specific collection interface.
+                Specifying <code>List</code> is unnecessary here, because we aren't using any <code>AList</code>-specific methods (like <code>get(0)</code>).
+                Instead of using the concrete definition, it is better to do something like:<br/>
+<pre><code>
+private void printList(Collection&lt;String&gt; list) {
+    ...
+</code></pre>
+                If the design ever changes, e.g. a <code>Set</code> is used instead, this code won't have to change.
+
+            </p>
+
+            <p>IDEs tend to have tools to help generalize parameters.  For example, in Eclipse, the refactoring tool <a href="http://help.eclipse.org/luna/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Freference%2Fref-menu-refactor.htm">Generalize Declared Type</a> helps find an appropriate level of concreteness.</p>
+            ]]>
+        </Details>
+    </BugPattern>
+
 
 	<BugPattern type="LII_LIST_INDEXED_ITERATING">
 		<ShortDescription>Method uses integer based for loops to iterate over a List</ShortDescription>

diff --git a/src/main/java/com/mebigfatguy/fbcontrib/detect/OverlyConcreteParameter.java b/src/main/java/com/mebigfatguy/fbcontrib/detect/OverlyConcreteParameter.java
@@ -436,8 +436,9 @@ private void reportBugs() {
                     parm++; // users expect 1 based parameters
 
                     String infName = definers.keySet().iterator().next().getClassName();
+                    boolean isCollection = "java.util.Collection".equals(infName);
                     bugReporter.reportBug(
-                            new BugInstance(this, BugType.OCP_OVERLY_CONCRETE_PARAMETER.name(), NORMAL_PRIORITY)
+                            new BugInstance(this, isCollection ? BugType.OCP_OVERLY_CONCRETE_COLLECTION_PARAMETER.name() : BugType.OCP_OVERLY_CONCRETE_PARAMETER.name(), NORMAL_PRIORITY)
                                     .addClass(this).addMethod(this).addSourceLine(this, 0)
                                     .addString(getCardinality(parm) + " parameter '" + name + "' could be declared as "
                                             + infName + " instead"));

diff --git a/src/main/java/com/mebigfatguy/fbcontrib/utils/BugType.java b/src/main/java/com/mebigfatguy/fbcontrib/utils/BugType.java
@@ -209,6 +209,7 @@ public enum BugType {
 
     OC_OVERZEALOUS_CASTING,
     OCP_OVERLY_CONCRETE_PARAMETER,
+    OCP_OVERLY_CONCRETE_COLLECTION_PARAMETER,
     ODN_ORPHANED_DOM_NODE,
     OI_OPTIONAL_ISSUES_CHECKING_REFERENCE,
     OI_OPTIONAL_ISSUES_PRIMITIVE_VARIANT_PREFERRED,