findbugs возражает против анонимного внутреннего класса

Этот код:

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

вызывает нарушение в Sonar, отключая правило findbugs "SIC_INNER_SHOULD_BE_STATIC_ANON", которое имеет описание:

Этот класс является внутренним классом, но не использует встроенную ссылку на объект, который его создал. Эта ссылка делает экземпляры класса больше и может поддерживать ссылку на объект-создатель дольше, чем необходимо. Если возможно, класс должен быть превращен в статический внутренний класс. Поскольку анонимные внутренние классы не могут быть помечены как статические, для этого потребуется рефакторинг внутреннего класса, чтобы он стал именованным внутренним классом.

В самом деле? Разве это не очень придирчиво? Должен ли я действительно провести рефакторинг однострочного метода в анонимном внутреннем классе, чтобы сэкономить на дополнительной ссылке? В этом случае нет возможности удерживать ссылку дольше, чем необходимо.

Я не возражаю против этого, так как наши строго соблюдаемые стандарты кодирования - это "нарушения без сонара", но я сильно склонен аргументировать //NOSONAR здесь, поскольку imho извлечение однострочного метода к статическому внутреннему элементу делает код немного сложнее в реализации.

Что думают ява-пуристы?

Источник

4 ответа

Решение

Преобразование комментариев в ответ, прежде всего, может убедить меня, что наличие этого в качестве анонимного внутреннего класса может быть оправдано, даже если есть четкая техническая причина быть придирчивым к этому.

Тем не менее, я бы сказал: следуйте установленным вами правилам. Правила создают согласованность, и когда весь код написан одинаково, базовую часть кода легче понять в целом. Если какое-то правило плохо, отключите его везде.

Когда есть исключение, также необходимо объяснить, почему есть исключение: дополнительная умственная нагрузка для человека, читающего код, дополнительный элемент для обсуждения в обзоре кода и т. Д. Отключайте правила только в отдельных случаях, если вы можете утверждать, что это как-то исключительный случай.

Кроме того, я не уверен, что делать это, так как статический класс будет менее понятным, даже если он добавляет немного больше шаблона (и извините, если ниже не 100% правильный код, мой Java немного ржавый, не стесняйтесь предлагать редактирование):

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

И затем где-нибудь еще в файле вместе с другими статическими классами:

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());
    }
}
Источник

Ради полноты, я бы хотел добавить еще один вариант к уже предоставленным отличным ответам. Определите константу для 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);
}

Это экономит вам дополнительный класс, как в ответе Хайда. В противном случае ответ Хайда лучше, потому что он позволяет вам объявить Comparator быть сериализуемым (что так и есть, потому что у него нет состояния). Если Comparator не сериализуем, ваш TreeSet тоже не будет сериализуемым.

Источник

Здесь есть три решения, лучшее из которых вне вашего контроля:

  • Расширьте синтаксис Java:

    ... theSet = new static Comparator ...

  • Объявите и используйте статический класс, как описано.

  • Проигнорируйте предупреждение в этом одном случае:

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

Я предпочитаю первое, но это долго ждать, если когда-либо. Таким образом, я хотел бы пойти на последнее, прежде чем игнорировать правило для всего проекта. Выбор правила должен повлечь за собой небольшую боль, чтобы переопределить его; в противном случае это просто соглашение или предложение.

Источник

Только примечание: анонимные внутренние классы являются отличным способом утечки памяти, особенно при использовании в компонентах JEE. Что-то так просто:new HashMap<>() {{ put"("a","b"); }};

в bean-компоненте, аннотированном @javax.ejb.Singleton может привести к тому, что несколько экземпляров будут поддерживаться, так как эта карта сохраняет ссылку на bean-компонент

Источник
Другие вопросы по тегам