Comments 10
Тестировал первую версю анализатора PSV studio для java. Мой вывод — есть смысл использовать, только если на проекте до этого совсем не применялся статический анализ кода. В ближайшее время поделюсь результатами на стабильной версии этого анализатора.
Или он должен быть сильно лучше встроенного в IDEA, или Сонара?
Да, заметил то же самое. Если писать в IDEA, то 99% из того, что может найти PVS, находится встроенным анализатором. Конечно, я не говорю про большие проекты(с легаси) — для них есть смысл.
По опыту смотрю — какие бы умные анализаторы не были в руках разработчиков, будь то встроенный в IDE или нет, на него всё равно не смотрят. А, между тем, полезные вещи говорят! Я то сам всегда внимательно читаю, что пишет компилятор (в первую очередь) и анализаторы-помогатели (во вторую), а толку, если остальные не смотрят? Даже в открытых проектах.
Да что там анализаторы — на простейшие warning'и среды и компилятора часто не смотрят :-(
Против этого есть простое решение: собирать на try-ботах с -Werror.
Если не собралось — патч не принимать.
Если не собралось — патч не принимать.
Это годится, если вы великодушный пожизненный диктатор нового проекта, например. А что делать, если вы пришли в команду, смотрите на эти миллионы строк кода, а там треш, угар и содомия? Если добавить нужные опции компилятора вот прямо сейчас, то просто всё упадёт и всё (а чинить никто не будет, у вас тоже нет времени, а у других ещё и желания). А бизнесу нужно, чтобы собиралось и работало.
Ну это уже soft-skills. Тут придётся «продавать» всё это так же, как PVS-Studio «продаёт» с помощью вот этих вот статей на Хабре.
Прошерстить багтрекер, найти моменты, когда проигнорированные warning'и привели к реальным ошибка, составить для бизнеса калькуляцию «сколько денег и репутации мы потеряли из-за ошибок, которых могло бы и не быть» и так далее.
Да, тяжело, а кому сейчас легко? Если у вас нет проблем из-за того, что у вас миллионы warning'ов, то бизнес прав, а если есть — так он легко найдёт деньги, когда увидит сколько это ему стоит.
Прошерстить багтрекер, найти моменты, когда проигнорированные warning'и привели к реальным ошибка, составить для бизнеса калькуляцию «сколько денег и репутации мы потеряли из-за ошибок, которых могло бы и не быть» и так далее.
Да, тяжело, а кому сейчас легко? Если у вас нет проблем из-за того, что у вас миллионы warning'ов, то бизнес прав, а если есть — так он легко найдёт деньги, когда увидит сколько это ему стоит.
Каждый раз, когда читаю ваши отчёты, у меня возникает ощущение, что софт сейчас вообще не должен работать, но каким-то чудом работает.
Критиковать других легко, знаю. Не всё же…
Есть ощущение, что человек, писавший код Elasticsearch (ну или конкретно данные куски приведенного кода), пришел из мира С/С++ и не совсем хорошо знал Java. Что навело на эти мысли?
Шаблоны из мира С:
1. Частое использование static
2. Предпочтение массивов использованию классов/коллекций; вообще слишком частое использование массивов.
3. Путаница == и equals.
4. Паттерн преобразования одного примитивного типа к другому (int)x.
5. Работа с текстом, не как с объектом, а как с массивом. for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++), или line.getBytes(StandardCharsets.UTF_8);
Вообще чувствуется дух C/C++. Процедурный подход превалирует над объектным.
Довод в пользу плохого знания Java:
1. сравнение line.startsWith("--" + boundary) c false не требуется, т.к. startsWith возвращает логическое значение.
2. игнорирование библиотечных функций Java. Например в методе withRestUriAndMethod можно было бы воспользоваться классом URL/URI, чтобы извлечь фрагмент. На автор кода решил, похоже, изобрести свой велосипед.
3. Инструкцию for (int i = 0; i < values.length; i++) можно было заменить на более понятный итератор for (Object value: values), избавившись от необходимости каждый раз писать values[i] instanceof. Вообще весь блок цикла в setSortValues, стоило вынести в отдельную функцию isPrimitiveClass и написать на неё тест.
4. Если уж используешь Objects.equals, то левая часть бессмысленна в условиях вида (frequency == null || Objects.equals(frequency, datafeed.getFrequency())
Ну и по мелочи:
1. Несколько смущает конструкция context.instanceClazz. Тоже самое: printStackTrace.
2. Похоже, что разработчик не пишет юнит-тесты, а если и пишет, то не запускает. Иначе ошибки в char64, findNextWhiteSpace всплыли бы сразу.
3. От методов findNextWhiteSpace/skipWhiteSpace стоит отказаться в пользу специализированного итератора. На такой итератор легче написать тест (чем на закрытый член private static). Плюс избавляемся от паттерна «GodObject».
4. Вообще наличие static-функций не в утилитарном библиотечном Java классе — явный сигнал к рефакторингу класса.
Есть ощущение, что человек, писавший код Elasticsearch (ну или конкретно данные куски приведенного кода), пришел из мира С/С++ и не совсем хорошо знал Java. Что навело на эти мысли?
Шаблоны из мира С:
1. Частое использование static
2. Предпочтение массивов использованию классов/коллекций; вообще слишком частое использование массивов.
3. Путаница == и equals.
4. Паттерн преобразования одного примитивного типа к другому (int)x.
5. Работа с текстом, не как с объектом, а как с массивом. for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++), или line.getBytes(StandardCharsets.UTF_8);
Вообще чувствуется дух C/C++. Процедурный подход превалирует над объектным.
Довод в пользу плохого знания Java:
1. сравнение line.startsWith("--" + boundary) c false не требуется, т.к. startsWith возвращает логическое значение.
2. игнорирование библиотечных функций Java. Например в методе withRestUriAndMethod можно было бы воспользоваться классом URL/URI, чтобы извлечь фрагмент. На автор кода решил, похоже, изобрести свой велосипед.
3. Инструкцию for (int i = 0; i < values.length; i++) можно было заменить на более понятный итератор for (Object value: values), избавившись от необходимости каждый раз писать values[i] instanceof. Вообще весь блок цикла в setSortValues, стоило вынести в отдельную функцию isPrimitiveClass и написать на неё тест.
4. Если уж используешь Objects.equals, то левая часть бессмысленна в условиях вида (frequency == null || Objects.equals(frequency, datafeed.getFrequency())
Ну и по мелочи:
1. Несколько смущает конструкция context.instanceClazz. Тоже самое: printStackTrace.
2. Похоже, что разработчик не пишет юнит-тесты, а если и пишет, то не запускает. Иначе ошибки в char64, findNextWhiteSpace всплыли бы сразу.
3. От методов findNextWhiteSpace/skipWhiteSpace стоит отказаться в пользу специализированного итератора. На такой итератор легче написать тест (чем на закрытый член private static). Плюс избавляемся от паттерна «GodObject».
4. Вообще наличие static-функций не в утилитарном библиотечном Java классе — явный сигнал к рефакторингу класса.
Sign up to leave a comment.
PVS-Studio for Java отправляется в путь. Следующая остановка — Elasticsearch