Вступление
На своем выступлении на DevCon - Практика применения Code-review получил запросы посмотреть, как в реальности это все проводится. Ведь интересно узнать не только о том, какие инструменты и как их применять, а увидеть живой пример проведения Code-review. Кажется, это отличная идея!
Результат - видеоролики с процессом Code-review и статьи на Инфостарт с результатами.
Для проведения Code-reivew планирую брать открытые решения на GitHub.
Исходные данные
1. Открытая обработка - Хранилище 1С. Просмотр истории хранилища обработкой от Anton Ivanov
2. Редактор - Visual Studio Code
3. Плагин - SonarQube 1C (BSL) Community Plugin
4. База знаний - Система стандартов и методик разработки конфигураций 1С
Результат
1. К статье приложены файлы с результатами код-ревью.
Замечания по коду отмечены // +++
2. Подробнее про процесс проведения и файлы с результатами Code-review размещены в репозитории - 1CodeReviewExample
Подготовка к ревью
Code-review будем проводить в Visual Studio Code, так как у него есть подсветка синтаксиса и поддержка правил от сообщества. Плюс нам не потребуется запускать код, мы будем его только смотреть и пытаться понять, что в нем происходит.
Обычно я провожу Code-review в два прохода:
1. Первый проход используется, чтобы отсеять слишком явные замечания и в принципе бегло ознакомиться, с чем имеем дело. В таком проходе обычно выявляются какие-то замечания к оформлению или явные нарушения стандартов. Замечания к логике или как можно сделать это проще не делается.
2. Второй проход уже смотрим внимательно, пытаясь понять, что в этом коде происходит и все ли сделано понятно и оптимально. На этом этапе уже можно найти какие-то фундаментальные замечания, например, что не учтен какой-то случай или используется неправильный алгоритм. Этот этап более энергозатратный, поэтому если на первом этапе было много замечаний, то до второго этапа с первого раза может и не дойти.
Процесс работы и примеры замечаний
В нашем примере начинаем проводить ревью начиная с формы, потому что там кода поменьше. В рамках примера мы рассматриваем только модули формы и модуля объекта, на сами формы не смотрим. Хотя в реальной жизни формы следует тоже смотреть. Сейчас готовлю отдельный Гайд по созданию форм на 1С.
Вот так выглядит наш модуль формы:
В целом код выглядит нормально, явных претензий нет. В глаза сразу бросается отсутствие Областей. Пишем такое замечание "// +++ Стоит добавить области". Замечания в целом к модулю добавляю вверху файла, чтобы их сразу было видно.
Также обращаем внимание на неканоническое написание ключевых слов, например "Конецесли" и "тогда". Следует писать "КонецЕсли" и "Тогда". Пишем также об этом замечание. Кажется, что достаточно написать это замечание в одном месте, упомянув, что в других местах тоже следует поискать. Если что-то будет пропущено автором - не страшно, сможем указать на это при следующем раунде Code-review.
Замечание пишем так: "// +++ Здесь и далее неканоничное написание ключевых слов. См. КонецЕсли и Тогда"
На этом первый проход будет завершен, так как больше ничего не бросается в глаза.
Приступаем ко втором проходу. Для этого внимательно читаем код и пытаемся понять, что он делает.
В принципе код простой и понятный и ничего особенного не замечаю.
Кроме одного момента:
Процедура УстановитьОтборВТЧ()
пВерсия = 0;
СтрокаИсторияХранилища = Элементы.ИсторияХранилища.ТекущиеДанные;
Если СтрокаИсторияХранилища <> Неопределено тогда
пВерсия = СтрокаИсторияХранилища.Версия;
Конецесли;
Элементы.ИзмененныеОбъекты.ОтборСтрок = Новый ФиксированнаяСтруктура("Версия", пВерсия);
КонецПроцедуры
Судя по коду у нас может быть ситуация, когда ничего не выбрано. Возможно, в этом случае либо показывать весь список, либо пустой и подсказку. Давайте это и напишем:
Процедура УстановитьОтборВТЧ()
пВерсия = 0;
СтрокаИсторияХранилища = Элементы.ИсторияХранилища.ТекущиеДанные;
Если СтрокаИсторияХранилища <> Неопределено тогда
пВерсия = СтрокаИсторияХранилища.Версия;
Конецесли;
// +++ Возможно, когда ничего не выбрано, стоит показывать весь список без отбора, а не по 0 версии.
Элементы.ИзмененныеОбъекты.ОтборСтрок = Новый ФиксированнаяСтруктура("Версия", пВерсия);
КонецПроцедуры
В остальном код, кажется, понятный и можно приступать к модулю объекта обработки.
Сначала первый проход с замечаниями, которые бросаются в глаза, например:
1. Не каноничное написание ключевых слов
2. Использование конкантенации строк вместо СтрШаблон
3. Экономия на пробелах. Например, не ставим их вокруг = или ,
4. Слишком длинные строки
5. Не используем ЗаполнитьЗначениеСвойств
и так далее
Примеры таких замечаний:
Приступаем ко второму проходу, вникая в то, что делает код в каждой процедуре.
Подробные размышления можно увидеть в дополнительных материалах в репозитории 1CodeReviewExample, а результат посмотреть в файлах к статье.
Здесь приведу лишь один пример размышления и замечаний для упрощения функции.
Например, увидел частое использование повторяющегося кода вот такого вида:
ПараметрыОтбора=Новый Структура();
ПараметрыОтбора.Вставить("Версия",пВерсия);
НайденныеСтрокиИзмененныеОбъекты = ИзмененныеОбъекты.НайтиСтроки(ПараметрыОтбора);
ВсегоНайденныеСтроки = НайденныеСтрокиИзмененныеОбъекты.Количество();
ТекстОшибки = "";
Если ВсегоНайденныеСтроки = 0 Тогда
ТекстОшибки = "Ошибка! Не найдена строка";
Конецесли;
Если ЗначениеЗаполнено(ТекстОшибки) Тогда
ТекстОшибки = ТекстОшибки
+" в ""ИзмененныеОбъекты"" для ";
Для каждого ЭлементОтбора из ПараметрыОтбора цикл
ТекстОшибки = ТекстОшибки
+" "+ ЭлементОтбора.Ключ + " = "+ЭлементОтбора.Значение;
Конеццикла;
ВызватьИсключение ТекстОшибки;
Конецесли;
В этой части кода мы пытаемся найти строки таблицы по структуре и если ничего не нашли, то выводим исключение.
Кажется, код можно упростить для понимания и чтения.
Например, мы видим отдельное условие для заполненного ТекстОшибка. Но это возможно, только если выполняется условие выше, когда ВсегоНайденныеСтроки = 0. То есть мы можем просто избавиться от второго условия объединив его с первым. Например, вот так:
ПараметрыОтбора=Новый Структура();
ПараметрыОтбора.Вставить("Версия",пВерсия);
НайденныеСтрокиИзмененныеОбъекты = ИзмененныеОбъекты.НайтиСтроки(ПараметрыОтбора);
ВсегоНайденныеСтроки = НайденныеСтрокиИзмененныеОбъекты.Количество();
ТекстОшибки = "";
Если ВсегоНайденныеСтроки = 0 Тогда
ТекстОшибки = "Ошибка! Не найдена строка";
ТекстОшибки = ТекстОшибки
+" в ""ИзмененныеОбъекты"" для ";
Для каждого ЭлементОтбора из ПараметрыОтбора цикл
ТекстОшибки = ТекстОшибки
+" "+ ЭлементОтбора.Ключ + " = "+ЭлементОтбора.Значение;
Конеццикла;
ВызватьИсключение ТекстОшибки;
Конецесли;
Смотрим на код еще раз и видим, что переменная ТекстОшибки у нас составляется из разных частей, с помощью конкантенации строк. Кажется, что проще использовать для этого СтрШаблон. Причем цикл для перебора параметров отбора тоже, кажется, излишним, так как мы сами вверху создали структуру всего с одним свойством. Поэтому можно явно указывать это свойство структуры. Код может выглядеть вот так:
ПараметрыОтбора=Новый Структура();
ПараметрыОтбора.Вставить("Версия",пВерсия);
НайденныеСтрокиИзмененныеОбъекты = ИзмененныеОбъекты.НайтиСтроки(ПараметрыОтбора);
ВсегоНайденныеСтроки = НайденныеСтрокиИзмененныеОбъекты.Количество();
ТекстОшибки = "";
Если ВсегоНайденныеСтроки = 0 Тогда
ТекстОшибки = СтрШаблон("Ошибка! Не найдена строка в ""ИзмененныеОбъекты"" для Версия = %1", ПараметрыОтбора.Версия);
ВызватьИсключение ТекстОшибки;
Конецесли;
Что еще здесь можно сделать проще? Например, можно отказаться от инициализации переменных - ВсегоНайденныеСтроки и ТекстОшибки:
ПараметрыОтбора=Новый Структура();
ПараметрыОтбора.Вставить("Версия",пВерсия);
НайденныеСтрокиИзмененныеОбъекты = ИзмененныеОбъекты.НайтиСтроки(ПараметрыОтбора);
Если НайденныеСтрокиИзмененныеОбъекты.Количество() = 0 Тогда
ТекстОшибки = СтрШаблон("Ошибка! Не найдена строка в ""ИзмененныеОбъекты"" для Версия = %1", ПараметрыОтбора.Версия);
ВызватьИсключение ТекстОшибки;
Конецесли;
Ну и последним шагом сразу инициализируем структуру и приведем код к каноничному виду.
ПараметрыОтбора = Новый Структура("Версия", пВерсия);
НайденныеСтрокиИзмененныеОбъекты = ИзмененныеОбъекты.НайтиСтроки(ПараметрыОтбора);
Если НайденныеСтрокиИзмененныеОбъекты.Количество() = 0 Тогда
ТекстОшибки = СтрШаблон("Ошибка! Не найдена строка в ""ИзмененныеОбъекты"" для Версия = %1", ПараметрыОтбора.Версия);
ВызватьИсключение ТекстОшибки;
КонецЕсли;
Таким образом, мы упростили код. Сравнение двух частей кода, которые делают одно и то же:
Подобным пристальным образом смотрим на остальной код, пытаясь понять что в нем делается и как это можно сделать проще. Записываем подобные замечания в файлик.
Итоги
Полный результат ревью и все замечания можно посмотреть в приложенных к статье файлах. Все замечания добавлены комментарием // +++, можно сразу по ним поискать в коде.
Результаты ревью направлены разработчику, кстати, не забудьте поставить звездочку обработке Хранилище 1С. Просмотр истории хранилища обработкой от Anton Ivanov.
Обычно после первого раунда Code-review разработчик исправляет замечания и присылает на повторное ревью и так до тех пор, пока ревьюер не примет код. Кстати, ревьюер затем также и делит ответственность за этот код с разработчиком. В случае если будет какая-то проблема, то будут вопросы и как допустили, и почему не нашли при ревью и тестировании. Поэтому важно делать ревью осознанно.
В ближайшее время планирую делать новые разборы кода.
Полезные ссылки
- Стек технологий для 1С
- Инструменты и процессы разработки 1С:Бухгалтерии
- Шаблоны новых объектов для 1С:Бухгалтерии
