Java findbugs 反对匿名内部类

findbugs is objecting to anonymous inner class

提问by Steve Atkinson

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:

在 Sonar 中触发违规，触发 findbugs 规则“SIC_INNER_SHOULD_BE_STATIC_ANON”，其描述如下：

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 //NOSONARhere, as imho extracting a one line method to a static inner makes the code slightly harder to grok.

我不介意这样做，因为我们强制执行的编码标准是“零声纳违规”，但我很想为//NOSONAR这里争论这个案例，因为恕我直言，将单行方法提取到静态内部会使代码稍微难以理解格罗克。

What do the java purists think?

爪哇纯粹主义者怎么看？

采纳答案by hyde

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 exceptionalcase.

当出现异常时，还需要解释为什么会出现异常：阅读代码的人有额外的心理负担，代码中需要讨论的额外项目等。只有在个别情况下禁用规则，如果你能争辩说它在某种程度上是一个特例。

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):

另外，我不确定这样做是因为静态类不太容易理解，即使它添加了更多样板（如果下面不是 100% 正确的代码，抱歉，我的 Java 有点生疏，请随时建议编辑） ：

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());
    }
}

回答by David Harkness

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 ...
  

• 扩展 Java 语法：

  ... theSet = new static Comparator ...
  

• 声明并使用静态类，如所述。

• 在这种情况下忽略警告：

  @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.

我更喜欢第一个，但如果有的话，那还需要很长时间。因此，在忽略整个项目的规则之前，我会选择最后一个。选择一个规则应该有点痛苦来覆盖它；否则它只是一个约定或建议。

回答by barfuin

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:

为了完整起见，我想在已经提供的优秀答案中添加另一个变体。为 定义一个常量Comparator，并使用它：

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 Comparatorto be serializable (which it is, because it has no state). If the Comparatoris not serializable, your TreeSetwon't be serializable either.

这为您节省了额外的课程，如海德的回答。否则，海德的答案更好，因为它允许您将 声明Comparator为可序列化（确实如此，因为它没有状态）。如果Comparator不可序列化，则您TreeSet也无法序列化。

回答by user158037

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"); }};

请注意：匿名内部类是泄漏内存的好方法，尤其是在 JEE bean 中使用时。很简单的事情： 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.

在用 @javax.ejb.Singleton 注释的 bean 中可能会导致多个实例保持活动状态，因为该 Map 保持对 bean 的引用。