mebigfatguy / fb-contrib

a FindBugs/SpotBugs plugin for doing static code analysis for java code bases
http://fb-contrib.sf.net
GNU Lesser General Public License v2.1
157 stars 45 forks source link

fb-contrib:PMB_POSSIBLE_MEMORY_BLOAT should not appear where there is no option to mutate the collection #472

Open mistriel opened 3 months ago

mistriel commented 3 months ago

my case has two issues related to this check,

  1. The docs appears to refer to collection or StringBuffer but not to a Map
  2. The warning should not appear where there is no option to bloat.

In the following scenario if putUnique was public there were an option to bloat the Map, yet since putUnique is private and roleMapping is private then roleMapping field is immutable.


public class Example{

    private static final Map<String, String> roleMapping = new HashMap<>();

    static {
        putUnique("Jhon", "User");
        putUnique("Kate", "Manager");
        putUnique("Gorge", "Supervisor");

    }

    public String getRole(String first) {

        return roleMapping.get(first);
    }

    private static void putUnique(String first, String last ) {
        String previous = roleMapping.put(first, last);
        if (previous != null) {
            throw new IllegalArgumentException("Duplicate key");
        }
    }
}
mebigfatguy commented 1 month ago

true. the issue is putUnique could be called elsewhere, so the question is how to determine where to stop? private? protected, package private??? etc. what if another method in this class called putUnique? What if a chain of 5 methods in this class called putUnique, and the last one was public? I'm not arguing you are correct, just that it's kind of untractable.

We could say a private method that is only called from a static block is ok. That's probably doable.

mistriel commented 3 weeks ago

Hi, So lets agree on the final statement, with addition to constructor as well.
WDYT ?

mebigfatguy commented 3 weeks ago

sure, assuming the field is an instance field