Sonar: пишем качественный код, или как осуществлять pre-commit анализ

ArtOfLife · 31.Декабрь.2014 15:03:04

В данной статье мы поговорим о важности написания качественного кода. Многие ошибочно считают, что код, создаваемый автоматизаторами, не должен проходить процесс ревью. Мол, это ведь не продакшен код, зачем его ревьювить? На это могу сразу задать встречный вопрос: а какая разница, продакшен он или нет?

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

Если копнуть глубже и провести поверхностный root cause analysis, то причиной данного распространенного заблуждения могут служить: либо синдром бога, мол - я пишу код, и он весь такой классный, в нем не может быть ошибок по определению; либо боязнь быть раскритикованным, мол - я новичок, еще многого не знаю, а тут меня сейчас какахами закидают, появятся комплексы и я вообще брошу автоматизировать.

Тут следует усвоить 2 правила:

Не бывает идеального кода. Баги можно найти везде. Посему нужно привлекать независимых экспертов, которые смогут более трезво оценить ваш код.
Код ревью направлен прежде всего на улучшение качества кода, а не на унижение реквестера. Не стоит воспринимать комментарии на свой личный счет. Все мы учимся. И это - один из способов саморазвития.

Итак, хватит лирики. Дальше мы поговорим о том, как проводить анализ своего кода до того, как отправлять его на ревью, создавая pull requests. Такой себе pre-commit analysis.

Применительно к Java, мы можем воспользоваться инструментом под названием SonarQube, представляющим из себя open source платформу для осуществления контроля качества кода.

В двух словах, процесс выглядит следующим образом: запускается анализатор кода (к примеру, посредством sonar maven plugin), покрывающий следующие code quality оси:

Результаты отправляются Sonar серверу, где вы сможете ознакомиться с найденными проблемами и подсказками к их решению.

Рассмотрим сей продукт на реальном примере из нашей базы знаний. Специально взял свой собственный код, чтобы показать, что:

я тоже человек и допускаю ошибки;
я не боюсь процесса код ревью.

Другой вопрос: как код с ошибками попал в мастер репозиторий? Естественно, речь идет не об ошибках компиляции. Но, да, да, я - редиска, создал PR без предварительного Sonar анализа. В оправдание ревьюверов могу только сказать, что некоторые из рассмотренных ниже ошибок не так легко распознать.

Итак, приступим к анализу моего кода.

Первым делом необходимо установить SonarQube 4.5.1 (LTS) (на момент чтения версия может быть другой). Скачиваем архив.
Распаковываем в любое удобное место.
Шагаем в bin folder.
Открываем директорию, соответствующую вашей системе.
Запускаем Start Sonar.bat (WIN).

В результате должно появиться нечто вроде этого:

Проверяем, что сервер запущен. Вбиваем в браузер localhost:9000 (дефолтный адрес) и видим следующее:

Предположим, что исходники проекта вы уже выкачали. Теперь создадим 2 maven run configuration:

Для корректного анализа Sonar требует предварительного запуска install goal, иначе получите кучу class loader ворнингов. Детальнее сможете прочесть в официальной документации.

В процессе исполнения sonar:sonar goal, плагин проверит, запущен ли сервер на дефолтном хосте (можно задать кастомный адрес). В случае успеха, ваш код будет жестко разобран и раскритикован, а результаты смогут наблюдать все, у кого есть доступ к вашему серверу.

Конечно в идеале сервер должен быть поднят на каком-то независимом окружении, к которому будут иметь доступ все члены команды. Но это уже out of scope.

Итак, представим, что мы все сделали верно, тогда в консоли можно будет наблюдать следующее:

Шагаем на дешбоард нашего сервера и находим наш проект:

Воу-воу-воу, сразу бросается в глаза technical debt - 1h 44min. Значит не все так идеально, как казалось, не так ли? За нас уже и посчитали приблизительное время на фикс найденных ошибок.

Раскрыв детали, можно увидеть множество полезной статистики. Но внимание следует все же уделить выделенным блокам:

Sonar уже распределил найденные ошибки по важности. Major severity по идее должно нас сильно испугать, посему за фикс именно этих багов мы первым делом и беремся.

Раскрыть подробную статистику ошибок можно либо посредством клика по соответствующей линке (кол-во), либо путем выбора меню Issues Drilldown.

Данный пейдж является основным для разбора полетов. Помимо severity, сразу бросается в глаза группировка по правилам (по сути саммари ошибок), клик по которым дает нам возможность увидеть список классов, в которых данная ошибка присутствует. Раскрыв сам класс, можно увидеть более точную информацию по проблемному месту.

Итак, идем по порядку:

Скобочки, как мы знаем, должны идти в соответствии с правилами языка. Даже если тело пустое. К счастью, в IntelliJ IDEA это решается путем вызова code reformat диалога: CTRL + ALT + L, Enter и вуаля - 2 бага пофикшено!

Возвращаясь к деталям, видим во-первых, что прогнозируемое время фикса (Debt: 1min) - более или менее точноe. Во-вторых, подсказки к решению настолько очевидны, что вам даже не надо ничего гуглить, чтобы решить данную проблему. Конечно далеко не всегда решения будут столь тривиальны, но согласитесь, что на таких вещах можно легко учиться!

Стоит отметить, что правила, которые используются для анализа вашего кода, могут кастомизироваться. К примеру, у нас в организации есть свои code style шаблоны. Sonar не обойдет стороной ваши policy. Вам необходимо лишь загрузить нужные темплейты.

Поехали дальше.

В Email классе мы видим целых 4 ошибки, связанные с несоблюдением java name convention - created_at филд с сопутствующими геттером / сеттером - явно не пример для подражания. Но как же быть? Ведь поле json’а, возвращающего структуру письма, содержит именно такое имя. Если мы его изменим, филд попросту не будет найден. Но выход есть! Путем явного задания аннотации @JsonProperty, мы сможем и парсер удовлетворить, и правила соблюсти.

Следующие 2 ошибки говорят о том, что если мы ловим эксепшены, то хорошо бы их логгировать или выплевывать наружу:

Не беда! Добавляем логгирование.

Далее немного сложнее:

Количество условных операторов, используемых в 1 выражении, велико. Решаем путем декомпозиции:

Но здесь же еще одна ошибка:

Тут нам советуют использовать готовые API проверки коллекции на пустоту. Оптимизируем обе строки с учетом декопмозиции:

Последний баг у нас относится к дублированию строк:

Выносим в константу и правим вхождения:

Ну что, пожалуй пора перезапустить анализатор и посмотреть результаты.

О чудо, technical debt = 0. И пусть теперь только попробуют придраться к моему PR.

В заключении отмечу, что Sonar - не панацея от всего. Доменных ошибок, связанных с логикой работы ваших тестов, этот инструмент конечно же не выявит. Но confidence в качестве вашего кода стремительно возрастет как у вас, так и ваших коллег / ревьюверов.

Wish you code quality improvement and Happy New Year!

Taras · 01.Январь.2015 21:57:01

когда то помниться читал книгу Clean Code Роджер Мартин…очень полезная штука для таких тем.

SUNx2YCH · 11.Январь.2015 15:22:59

Было:

return emails != null && emails.size() > 0 ?
        emails.get(index >= 0 && index < emails.size() ? index : 0) : null;

Стало:

final int updatedIndex = emails != null && index >= 0 && index < emails.size() ? index : -1;
return updatedIndex > -1 ? emails.get(updatedIndex) : null;

Сергей, вы действительно считаете, что код стал лучше, раз сонар перестал “ругаться”?
Мне кажется, вы забываете про основное правило :

Always code as if the person who ends up maintaining your code is a violent psychopath who knows where you live.

Теперь по пунктам:

Иногда тернарные операторы упрощают код и повышают его “читабельность”. Имхо, это не тот случай, особенно учитывая сложные условия и вложенность (в исходном варианте).
Метод getEmails() не должен возвращать null вместо ожидаемого List<Email>, это моветон. Если не можем получить письма, бросаем исключение или пишем ошибку в лог и возвращаем пустую коллекцию. В данном случае, наверное, лучше бросить исключение.
index >= 0 это отдельная проверка на валидность входных данных, не стоит мешать ее в одну кучу с остальными. Если нам передали некорректный индекс, можно бросить стандартный IllegalArgumentException.

С учетом вышесказанного можно получить примерно такой метод:

public Email getEmailByIndex(final int index) {

    if (index < 0) {
        throw new IllegalArgumentException("Incorrect index: " + index);
    }

    final List<Email> emails = getEmails();
    // обратите внимание на "лишние" скобки
    return (index < emails.size()) ? emails.get(index) : null;
}

ArtOfLife · 11.Январь.2015 20:06:53

По поводу сложности чтения тернарного оператора конкретно для этого случая я бы поспорил, т.к. ничего сверхествественного там нет: стандартные проверки на null / range check.

getEmails никогда не вернет null. Если писем нет, вернется пустая коллекция. Проверку поставил на автомате, но спасибо за замечание.

Касательно индекса… О каких остальных проверках речь? Все использованные проверки - проверки входных данных (как было сказано выше - range check, с учетом лишнего NPE). Да и раз уж вы решили вынести index < 0 check отдельно, то почему для значений < 0 -> эксепшен, а для >= size -> null? Чем проверка правой границы так отличается от левой?

П.С. Любой код можно где-то упростить / оптимизировать. Суть темы была несколько в другом: как пользоваться Sonar’ом. И как уже было сказано выше - я тоже человек и могу допускать ошибки.

SUNx2YCH · 11.Январь.2015 21:13:16

Я понимаю, что тема немного не об этом. Но она называется “пишем качественный код” и содержит пример из базы знаний, которая позиционируется как источник “примеров для подражания” для начинающих автоматизаторов. Поэтому меня и зацепила подобная “оптимизация” для избавления от предупреждений сонара.

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

По поводу проверки индекса: некорректно только входное значение < 0. Представьте ситуацию, когда нам нужно получить последнее письмо (index = 0), но письмо по какой-то причине задержалось и ящик пуст в момент вызова метода. Если и бросать в таком случае исключение, то явно не IllegalArgumentException, т.к. входное значение 0 вполне корректно.

ArtOfLife · 11.Январь.2015 22:21:19

Пример лишь отражает концепцию работы с MailCatcher via REST. Вы ведь понимаете, что помимо лишней NPE check, ваши замечания - лишь субъективный взгляд на очевидные вещи?! К тому же, ваш пример совершенно ничего не оптимизирует: больше строк, сомнительный throw. Каждый понимает / смотрит на код по-своему.

Пример в БЗ был добавлен 4 месяца назад. Оригинал для реального использования на автомейшен проекте был написан еще задолго до этого. Никаких проблем с читабельностью или пониманием не возникало ни у меня, ни у других автоматизаторов (даже джунов). Извините, но “измерять” время на понимание 2х строк кода, возвращающего один объект по индексу, - вам самому то не смешно?

Откуда вы знаете, какое письмо - последнее - в случае с индексами? Особенно при параллельном запуске. Вообще не понимаю, зачем вам тут throw сдался? Так вот, ответьте на вопрос: что с точки зрения оптимизации выгодней - проверить Email на null или оборачивать вызов в try / catch? Вы ведь не API тестируете, а лишь выгребаете содержимое письма, не так ли?

Этот клиент с большой долей вероятности будет зашиваться в библиотеку. Какой % людей полезет “оптимизировать” рабочие API, в которых нет проблем с пифоменсом, только из-за того, что кому-то тяжело читать тернарные операторы?

Предлагаю закрыть это бессмысленное обсуждение. С радостью ждем ваши примеры в БЗ, либо PRы с фиксами.

SUNx2YCH · 11.Январь.2015 22:40:37

Ок, если всё, что вы заметили, это “больше строк”, то вряд ли я вам что-то докажу. Удачи.

ArtOfLife · 12.Январь.2015 08:03:45

Так вы собственно и не привели никаких аргументов, чем ваш код оптимальнее. Давайте везде пробрасывать эксепшены! Особенно там, где их можно избежать. Зачем нам error codes? Или давайте раскрывать “глубокий” смысл if (isValid) через if (isValid == true). Так же легче читать, особенно начинающим автоматизаторам!

SUNx2YCH · 12.Январь.2015 09:18:30

Я просто оставлю это здесь:

P.S. обе есть на русском языке, могу скинуть в личку

sidelnikovmike · 12.Январь.2015 11:00:09

А вот кстати можно поинтересоваться, чем if(isValid) “качественнее” чем if(isValid == true)? или проверка isEmpty вместо size? Что это за странная “качественность”? Ведь пост называется именно “пишем качественный код”. Просто интересно услышать мысли
Одно дело - java code conventions, но тут - кажется это дело каждого, как писать. И то, что sonar считает это “плохим кодом” - это точно не показатель, что он именно такой.
Ну и лично моё мнение - тернальные операторы это вообще зло злющее. И уж куда понятнее код становится без них.

ArtOfLife · 12.Январь.2015 11:45:52

Как все любят тыкать пальцем в книжки, в которых приводятся примеры с гораздо более сложной логикой, где это действительно обоснованно. Мы уже с вами определили, что ввиду лишней NPE check, метод прекрасно превращается в примитивный:

final List<Email> emails = getEmails();
return index >= 0 && index < emails.size() ? emails.get(index) : null;

Ввиду того, что мы тестируем не MailCatcher API, а лишь выгребаем email content, нам важны всего 2 состояния: письмо либо есть, либо его нет, т.е. Email vs null. Такой код пишется один раз и забывается, т.к. точка входа у него будет всего 1 - EmailUtils тестового фреймворка. А ввиду того, что пробросы подобных эксепшенов (без соответствующей обработки) могут зафейлить весь тест, который тестирует совсем не API smtp сервера, то exceptions handling для данного конкретного примера “защиты от дурака” - чистейший оверхэд из серии “я в книжке прочитал - это не по фен-шую”.

Это был стёб на тему сложности чтения определенных конструкций языка.

Сонар не считает это “плохим кодом”. Сонар, впрочем, как и IntelliJ IDEA считает, что этот код можно упростить, т.е. записать гораздо короче. В таком случае, зачем писать длиннее, считая, что якобы кому-то (кому?) это будет понятней? Это из серии, сколько времени уйдет на написание System.out.println(""); vs sout, или public static void main(String[] args) {} vs psvm побуквенно?

Все это очень индивидуально. Кому-то новые фишки Java 8 тоже кажутся страшными и непонятными. Но через пару лет это будет рядовой синтаксис.