Новые правила "Одинаковые условия в последовательности вида Если (....) ИначеЕсли (....) ИначеЕсли (....)" и "Одинаковый код во взаимоисключаемых ветках ветвления кода"

sonar-bsl-plugin

#1

В следующем релизе будет новое интересное правило

Дублируются условия в разных ветках оператора “Если”.

Часто это является последствиями неверного или незаконченного “копипаста”.

Примеры неверных условий

Если Перем1 = Перем2 Тогда
    А = 1;
ИначеЕсли Перем1=Перем2 Тогда
    Б = 1;
КонецЕсли;
Если УсловиеВыполнено() Тогда
    А = 1;
ИначеЕсли Функция2(Значение1, Значение2) Тогда // ошибка
    Б = 1;
ИначеЕсли Функция2(Значение1, Значение2) Тогда // ошибка
    В = 1;
КонецЕсли;

или более сложный вариант дублирования

Если ПервоеУсловиеВыполнено() Тогда
    А = 1;
ИначеЕсли Перем1 = Перем2 Тогда // ошибка
    Б = 1;
ИначеЕсли ТретьеУсловиеВыполнено() Тогда
    В = 1;
ИначеЕсли Перем1 = Перем2 Тогда // ошибка
    Г = 1;
КонецЕсли;

Примеры срабатывания из типовых конфигураций


SonarQube 1C (BSL) - Выпуск релиза 1.13
#2

И еще одно правило начинает свою работу

Ух как 1С-ники любят копипаст

СППР - Справочник Задачи, форма ГрупповоеСозданиеЗадач

В методе аж 3 варианта (Если, ИначеЕсли и неявное Иначе) и везде одинаковый результат :slight_smile:


#3

Выше результат по новому правилу “Одинаковый код во взаимоисключаемых ветках ветвления кода”


#4

Или вот такой вот вариант :slight_smile:

Для оператора ?(Условие, Выражение1, Выражение2)


#5

Или намного более сложный пример, который глазами нелегко заметить


#6

Еще веселенькие условия, приводящие к одному результату :slight_smile:


#7

Еще большие совпадающие куски кода


#8

и еще


#9

Слушай. А ведь реально глазами фиг увидишь.


#10

Третья стрелка явно лишняя.


#11

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


#12

одно дело, когда вы осознанно добавляете такую денормализацию с целью улучшения читабельности кода. помечаете его как won’t fix (с комментарием) и идете дальше.
другое дело, когда это результат копи-паста и это вероятный баг, а не просто code smell.


#13

Баг - это когда работает не правильно, а не когда повтор кода.
Когда повтор кода - или говногод или так задумано.

Как вы себе представляете код, который переводит стейт-машину, допустим, из 5 условий, полностью раскрывающих СДНФ в 10 возможных состояний. Очевидно, что там неизбежны повторы. И очевидно, что уход от СДНФ приведет к меньшей читабельности кода (а иногда к большему количеству ошибок). И опять же, писать напротив каждого не уникального присвоения нового состояния комментарий, что оно не уникально, будет мешать чтению кода.

Мое мнение - именно эта часть правила слишком техногенна.

PS. Про стейт машину не надо начинать холивар про матрицы состояний :wink:


#14

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

машина состояний - это все же очень специфичный кейс для среднестатистического алгоритма 1с-ника.

я предлагаю вообще не холиварить :slight_smile: неугодное правило всегда можно отключить в собственной настройке профиля качества.


#15

Регистр правил - вполне себе хороший паттерн, попривыкли свитчить.


#16

Абсолютно согласен.
Копипаст часто приводит к различным ошибкам.
И дублирование кода самая мелкая из этих проблем.


#17

Новые красивые срабатывания, которые глазами не увидишь :slight_smile:


#18

Аж 5 раз код дублируется!


#19

Или еще немалый блок кода


#20

В чем конкретно тут major проблема? Одинаковые версии тут не для всех веток используются, что на мой взгляд вполне приемлемо. Хотелось бы увидеть ваш вариант рефакторинга :slight_smile:

Ладно бы это не 1С был, а что-то вроде C++, где switch + fallthrough есть.