findbugs is objecting to anonymous inner class

Question

This code:

Set<Map.Entry<String, SSGSession>> theSet =  new TreeSet<Map.Entry<String, SSGSession>>(new Comparator<Map.Entry<String, SSGSession>>() {

        @Override
        public int compare(final Map.Entry<String, SSGSession> e1, final Map.Entry<String, SSGSession> e2) {
            return e2.getValue().getStartTime().compareTo(e1.getValue().getStartTime());
        }
    }));

triggers a violation in Sonar, tripping the findbugs rule "SIC_INNER_SHOULD_BE_STATIC_ANON" which has the description:

This class is an inner class, but does not use its embedded reference to the object which created it. This reference makes the instances of the class larger, and may keep the reference to the creator object alive longer than necessary. If possible, the class should be made into a static inner class. Since anonymous inner classes cannot be marked as static, doing this will require refactoring the inner class so that it is a named inner class.

Really? Isn't this very nit-picky? Should I really refactor a one line method in an anonymous inner class to save the cost of an extra reference ? In this case, there's no possibility of it holding the reference longer than necessary.

I don't mind doing it as our strongly enforced coding standards are "zero sonar violations" but I'm strongly tempted to argue the case for a //NOSONAR here, as imho extracting a one line method to a static inner makes the code slightly harder to grok.

What do the java purists think?

Answer 1

Converting comments to answer, first of all I could be persuaded that having this as anonymous inner class can be justified, even if there's a clear technical reason for being nit-picky about this.

Still, I would say: Follow the rules you have set. Rules create consistency, and when all the code is written the same way, the code base is easier to understand as a whole. If some rule is bad, disable it everywhere.

When there is an exception, there's also need to explain why there's exception: an extra mental burden for someone reading the code, an extra item to discuss in code review, etc. Only disable rules in individual cases if you can argue it is somehow an exceptional case.

Also, I'm not sure doing it as static class would be less understandable, even if it adds a bit more boilerplate (and sorry if below is not 100% correct code, my Java is a bit rusty, feel free to suggest edit):

Set<Map.Entry<String, SSGSession>> theSet 
    = new TreeSet<Map.Entry<String, SSGSession>>(new SSGSessionStartTimeComparator());

And then somewhere else in the file, together with other static classes:

static class SSGSessionStartTimeComparator extends Comparator<Map.Entry<String, SSGSession>>() {
    @Override
    public int compare(final Map.Entry<String, SSGSession> e1, final Map.Entry<String, SSGSession> e2) {
        return e2.getValue().getStartTime().compareTo(e1.getValue().getStartTime());
    }
}

Answer 2

Just for completeness' sake, I'd like to add another variant to the excellent answers already provided. Define a constant for the Comparator, and use that:

private static final Comparator<Map.Entry<String, SSGSession>> BY_STARTTIME =
        new Comparator<Map.Entry<String, SSGSession>>() {
    @Override
    public int compare(final Map.Entry<String, SSGSession> e1,
            final Map.Entry<String, SSGSession> e2) {
        return e2.getValue().getStartTime().compareTo(e1.getValue().getStartTime());
    }
};

private void foo() {
    Set<Map.Entry<String, SSGSession>> theSet =
        new TreeSet<Map.Entry<String, SSGSession>>(BY_STARTTIME);
}

This saves you the additional class as in hyde's answer. Otherwise, hyde's answer is better, because it allows you to declare the Comparator to be serializable (which it is, because it has no state). If the Comparator is not serializable, your TreeSet won't be serializable either.

Answer 3

There are three solutions here, the best of which is out of your control:

• Extend the Java syntax:

  ... theSet = new static Comparator ...
  

• Declare and use a static class as described.

• Ignore the warning in this one instance:

  @SuppressFBWarnings("SIC_INNER_SHOULD_BE_STATIC_ANON")
  ... your method ...
  

I prefer the first, but that's a long time coming if ever. Thus I would go for the last before ignoring the rule project-wide. Choosing a rule should entail a little pain to override it; otherwise it's merely a convention or suggestion.

Answer 4

Just a note: anonymous inner classes are great way to leak memory, especially when used in JEE beans. Something as simple: new HashMap<>() {{ put"("a","b"); }};

in bean annotated with @javax.ejb.Singleton might lead to multiple instanced being kept alive as that Map keeps reference to bean.