From 6aedf19cbc713e3326bb34b2be0863d94c868f25 Mon Sep 17 00:00:00 2001 From: Dave Brosius Date: Tue, 10 Dec 2024 03:22:16 -0500 Subject: [PATCH] split out collection OCP issues into a separate category --- etc/bugrank.txt | 1 + etc/findbugs.xml | 3 +- etc/messages.xml | 35 +++++++++++++++++++ .../detect/OverlyConcreteParameter.java | 3 +- .../mebigfatguy/fbcontrib/utils/BugType.java | 1 + 5 files changed, 41 insertions(+), 2 deletions(-) diff --git a/etc/bugrank.txt b/etc/bugrank.txt index b0f7139f7..568e55646 100644 --- 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 index 7e31ccbe7..e565c06ae 100755 --- a/etc/findbugs.xml +++ b/etc/findbugs.xml @@ -41,7 +41,7 @@ - + @@ -349,6 +349,7 @@ + diff --git a/etc/messages.xml b/etc/messages.xml index b1d2c7935..ffd3727dc 100755 --- a/etc/messages.xml +++ b/etc/messages.xml @@ -1951,6 +1951,41 @@ private void appendToList(List<String> list) { ]]> + + + Method needlessly defines Collection parameter with concrete classes + {1}: {3} +
+ 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.

+ +

Take the following example:
+


+private void printList(List<String> list) {
+    for (String s : list) {
+        System.out.println(s);
+    }
+}
+
+ + The parameter list is currently defined as an List, which is a specific collection interface. + Specifying List is unnecessary here, because we aren't using any AList-specific methods (like get(0)). + Instead of using the concrete definition, it is better to do something like:
+

+private void printList(Collection<String> list) {
+    ...
+
+ If the design ever changes, e.g. a Set is used instead, this code won't have to change. + +

+ +

IDEs tend to have tools to help generalize parameters. For example, in Eclipse, the refactoring tool Generalize Declared Type helps find an appropriate level of concreteness.

+ ]]> +
+ + Method uses integer based for loops to iterate over a List diff --git a/src/main/java/com/mebigfatguy/fbcontrib/detect/OverlyConcreteParameter.java b/src/main/java/com/mebigfatguy/fbcontrib/detect/OverlyConcreteParameter.java index 6abd9f995..7005473bf 100755 --- 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 index a3bc58764..33f6d5046 100644 --- 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,