Впечатления от Eclipse-плагина PMD

    На днях был один пост о полезных плагинах для Eclipse от пользователя gAmUssA, в котором был упомянут в том числе и PMD. Мне этот плагин показался интересным и я решил по-пробовать натравить эту зверушку на свой проект…

    Для начала о проекте — это довольно не маленький java проект в 2000+ файлов кода. Так что изучить этому плагину было что. Писался и пишется он на протяжении уже около 4х лет и не менее чем 10 разными программистами, как нашими, так и забугорными.
    В специфику проекта вдаваться не буду — особого значения это сейчас не имеет.
    Ну в общем понеслась… Поставился он без проблем и лишних вопросов. Запустил я его и минут 5-7 он занимался глубоким анализом всего того бреда, который мы с коллегами годами писали. В итоге в Volations Overview View был выдан результат: такая себе табличка с пакеджами и классами в которых были найдены «ошибки». В тоже время в Volations Outline View можно было видеть описание этих «ошибок» и уже оттуда можно было перейти к конкретной строке кода.
    В плагине предусмотрено 5 приоритетов «ошибок»:
    1. Error High
    2. Error
    3. Warning High
    4. Warning
    5. Information

    С 3-го по 5-й слишком незначительны, что бы их упоминать и в большей степени относятся к спорного рода рекомендациям, вроде «A method argument that is never assigned can be declared final» (ну конечно он может быть объявлен финальным, но особого смысла в этом нет, как мне кажется), а также неиспользованным импортам.
    Поэтому я сразу выключаю отображение «ошибок» этих приоритетов (плагин легко позволяет это сделать нажатием соответствующей кнопочки в Volations Overview).
    Итак, остаются только самые страшные, с точки зрения PMD ошибки.
    Среди них я выделил наиболее распространение в моем коде. Учитывая объем этого кода, я считаю результирующие данные достаточно репрезентативной выборкой, чтобы говорить о близких закономерностях в других проектах. :)

    Наиболее часто встречающимися ошибками оказались следующие:
    1. An empty method in an abstract class should be abstract instead
      Мне кажется, спорное утверждение… Ну вот не нужно мне, что бы наследники в обязательном порядке переопределяли этот метод. Зачем я буду его делать абстрактным?
    2. System.out.print is used
      Тут конечно не поспоришь… Кто-то что-то отлаживал и забыл убрать. Стыдно. И это при наличии живого логгера.
    3. Overridable method called during object construction
      Это предупреждение очень часто встречается моем коде. И оно тоже вызывает большие сомнения. Понятно, почему стоит этого опасаться… Мол не успеем инициализировать объект и будем что-то дёргать и все посыпется. Но PMD ругается этим предупреждением на любой метод класса, вызванный из конструктора, будь то сеттер или нечто другое (конечно, если метод не приватный). То есть, с их точки зрения, в конструкторах должна присутствовать только прямая инициализация внутренних параметров плюс весь сопутствующий код. А если этого кода много? А если он не один раз используется? В общем если неукоснительно следовать этому правилу, то это солидно усложняет жизнь.
    4. Variable Naming Conventions
      Всем понятно, о чем речь и что неправильно называть переменные неправильно.:) Но бывают ситуации когда без этого тяжело обойтись. В моем случае чаще всего это оказывалось использование знака подчёркивания как префикса или суффикса. Но безусловно, этого нужно избегать.
    5. Avoid reassigning parameters
      (имеется ввиду реинициализация входных параметров метода внутри метода)
      С моей точки зрения это не красиво. Действительно, лучше уж пользоваться временными локальными переменными. Прозрачнее код.
    6. A class which only has private constructors should be final
      Ну и зачем? Что в одном, что во другом случае класс не наследуем. Что называется, одно другому не мешает. Может я что не понимаю?
    7. Do not use the short type
      Ну тут тяжело не согласиться. Пользуется он только для экономии памяти, но при вычислениях все равно конвертится в int и обратно. Так что я вижу смысл использовать только константы этого типа.
    8. Avoid instantiating Xxx objects...
      Где Xxx это String, Integer, Long, Boolean… У них у все есть valueOf(), а стрингу вообще ничего не надо. По большому счету — те же яйца, только в профиль. Единственное что, для обёрток числовых типов инициализация через valueOf() по их словам «is more memory friendly». Ну поверим.
    9. Do not explicitly trigger a garbage collection
      Не считаю прямой вызов сборщика мусора хорошим тоном, но все таки иногда приходится.


    Выводы



    Ну в принципе полезная вещь, что бы по-вылавливать мелких тараканов из кода. Но не более чем. Я не могу сказать, что результаты работы плагина меня как-то удивили или расстроили. Просто стало ещё раз понятно, что работаем не без огрехов и что есть к чему стремиться.
    Ещё хочется отметить, что есть возможность написания своих правил проверки кода. Лично попробовать не успел пока, но думаю там есть некоторое поле для фантазии.

    Спасибо за внимание. Надеюсь эта статья была кому-то полезной.

    Похожие публикации

    AdBlock похитил этот баннер, но баннеры не зубы — отрастут

    Подробнее
    Реклама

    Комментарии 27

      +2
      Забавно… Нарыл несколько ошибок в своем тексте, а поправить не могу — Возникла ошибка в получении XML данных:
      Internal Server Error
        +1
        Спасибо за статью, обязательно посмотрю плагин. Подобные вещи очень сильно помогают делать код более профессиональным.
          +1
          А зачем вызывать сборщик мусора? Ведь не факт, что он он будет вызван.
            +2
            яве всё равно, а разработчику приятно :)
              0
              Простите, не совсем понял Ваш вопрос.:)
                0
                Calling the gc method suggests to run garbage collection, not orders to
                  0
                  Просто немного запутанная формулировка.
                  «А зачем вызывать сборщик мусора вручную?»
                  и
                  «А зачем вызывать сборщик мусора, если он может быть не вызван?»
                  это два немного разных вопроса.:)
                    0
                    Зачем вообще вызывать сборщик мусора? И зачем его вызываете лично Вы, если никто не гарантирует, что он вызовется? :)
                      0
                      Лично я его не вызываю:) Но, в нашем продукте, он вызывается в тех местах, где в результате некоторых действий (построения некого формата, прохождения объёмных транзакций etc.) могут оставаться большие висячие куски данных в памяти, ожидающих следующие циклы сборки мусора. Вот и приходится заставлять jvm заниматься уборкой в форс мажорном порядке. Насколько я помню gc() не вызывается если при запуске установлена опция -Xdisableexplicitgc.
              +1
              Советую попробовать плагин с подобной функциональностью:
              checkstyle.sourceforge.net/
              eclipse-cs.sourceforge.net/

              Было бы интересно узнать, насколько отличаются результаты проверки кода у этих плагинов
                +1
                Спасибо. В свободное время обязательно попробую и поделюсь результатом.
                0
                очень много сомнительных предупреждений вроде «Avoid using if… else statements without using curly braces», имеющих отношение больше к субъективным представлениям о красоте кода, чем к реальным проблемам
                  0
                  Полностью с Вами согласен, что и пытался передать в статье. Но, опять же, такого рода «предупреждения» относятся 3-5 приоритетам. На них вообще внимания обращать не стоит, ИМХО.
                    +3
                    В основном такие плагины следуют букве Sun Code Conventions.
                    Это действительно вопрос красоты и удобства чтения.
                    Есть рекомендуемый стиль кода.

                    И лично я не вижу преимуществ у кода, который не соответствует SCC.
                    Другое дело, что иногда нету преимуществ у кода, который причесан под SCC.

                    Поэтому если плагин применяется под существующий код, то исправлять множество таких предупреждений смысла нету.
                    Но для нового кода плагин полезен. Сразу во время написания кода видишь предупреждения и исправляешь.
                    Времени это не отнимает совсем.
                      +2
                      Контроль следования «букве закона» SCC это только подмножество правил этого плагина.

                      С моей точки зрения этим правилам лучше следовать. Как минимум код становится читабельным. Поэтому такого рода плагины наверняка были бы полезны начинающим программистам, у которых только формируется стиль.
                    0
                      0
                      Есть.
                      Неплохая вещь, я у себя нашел несколько хороших предложений по улучшению качества кода.
                      Но, как и в случае с PDM, некоторые предупреждения обсуждаемы и их иногда хочется выключить. Особенно те, которые конфликтуют со стилем, в котором пишутся например Eclipse вещи. Но увы, фильтровать на уровне compilation unit'а плагин не помогает. В FindBugs кажется есть фильтры, но писать их руками неудобно.

                      Лично для меня, когда в проекте есть предупреждения я стараюсь их ВСЕ исправить. Просто чтобы всегда было чисто и всегда видел, что появилась новая проблема, которую нужно исправить. А в данном случае не получается. Что огорчает и заставляет отказать от использования, ибо стоимость просмотра всех сообщений каждый раз представляется выше, чем полученная выгода от полезных предложений.
                      0
                      PMD — это скорее рекомендация обратить внимание на потенциально ошибочный код, следовать ей или нет — это уже по ситуации. А за code convention-ом и т.п. должен следить checkstyle.
                      У нас на большинстве проектов по дефолту ставится и PMD и CheckStyle, + их статистика собирается автобилдами. Такой подход в больших проектах реально полезен.
                        0
                        Возможно он действительно реально полезен, но я с трудом представляю, как внедрить эту практику в уже существующий, солидный проект.
                          0
                          в существующий с большим количеством кода — гиблое дело. да и смысла особого нет — затраченное время не окупится результатом (разве что пройтись по самым грубым ошибкам)
                        +1
                        Пользовался им в IDEA (7+)
                        Чесьно говоря, мне встроенный в идею анализатор кода понравился гораздо больше.
                        И более информативный диагноз и упрощённое исправление (прямо тут можно нажать на линк и он исправит).
                          0
                          А вы не пользовались TPTP (eclipse.org/tptp)?

                          Интересно было бы узнать различия.
                            0
                            К сожалению нет, но я постараюсь найти время его по-пробовать.
                            Возможно позже выйдет сравнительная статья-анализ похожих плагинов.
                            0
                            Overridable method called during object construction

                            То есть, с их точки зрения, в конструкторах должна присутствовать только прямая инициализация внутренних параметров плюс весь сопутствующий код. А если этого кода много? А если он не один раз используется? В общем если неукоснительно следовать этому правилу, то это солидно усложняет жизнь.

                            Ну так если код дублируется в разных конструкторах одного класса, то его надо выносить в приватные методы, а если он дублируется в конструкторах подклассов, то вызывать родительские конструкторы, вызывающие эти приватные методы. Или я чего-то не так понял?
                              0
                              Речь не о дублируемости кода в конструкторах одного класса или разных классов одной иерархии. Просто если в конструкторе Вам понадобится вызвать метод xxx(), и этот метод будет не приватным — то есть будет необходимость его использовать из других точек кода — тогда PMD будет ругаться.
                              Например
                                0
                                *извините, глюк вышел
                                Так вот например, если в конструкторе кроме
                                this.a = a;
                                this.b = b;
                                нужна еще какая-либо инициализация, то с точки зрения PMD она должна быть либо вынесена в приватный метод, либо происходить прямо там. А это далеко не всегда оправдано.
                                0
                                В нашей компании статический анализатор кода (корпоративный) запускается на ночных билдах. В начале он нашел довольно много серьезных ошибок типа возможных NPE, незакрытых потоков, SQL injection'ов и т. д. Постепенно исправили. Только потом начали искать и править вещи типа {s = new String(«aaa»);} — из любви к искусству.

                                Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                                Самое читаемое