Пример проведения Code-review #1

04.06.24

Разработка - Рефакторинг и качество кода

В статье расскажу и покажу процесс проведения Code-review на примере обработки с GitHub.

Скачать файл

ВНИМАНИЕ: Файлы из Базы знаний - это исходный код разработки. Это примеры решения задач, шаблоны, заготовки, "строительные материалы" для учетной системы. Файлы ориентированы на специалистов 1С, которые могут разобраться в коде и оптимизировать программу для запуска в базе данных. Гарантии работоспособности нет. Возврата нет. Технической поддержки нет.

Наименование По подписке [?] Купить один файл
Результат проведения Code-review для модуля Формы
.bsl 3,29Kb
0
0 Скачать (1 SM) Купить за 1 850 руб.
Результат проведения Code-review для модуля Объекта
.bsl 44,64Kb
0
0 Скачать (1 SM) Купить за 1 850 руб.
Архив с модулями до и после проведения Code-review
.zip 15,59Kb
2
2 Скачать (2 SM) Купить за 2 150 руб.

 

Вступление

 

На своем выступлении на 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С
  2. Инструменты и процессы разработки 1С:Бухгалтерии
  3. Шаблоны новых объектов для 1С:Бухгалтерии

 

См. также

Рефакторинг и качество кода Программист Стажер Платформа 1С v8.3 Конфигурации 1cv8 Бесплатно (free)

В последнее время термин «чистый код» стал очень популярным. Появились даже курсы по данной тематике. Так что же это такое?

16.09.2024    14872    markbraer    65    

41

Рефакторинг и качество кода Программист Бесплатно (free)

В статье рассматривается отказ от использования процедур и унификация формата ответа функций. Способ описывается на примере развития абстрактной информационной системы, работающей с PDF файлами.

10.09.2024    1012    acces969    4    

6

Рефакторинг и качество кода Бесплатно (free)

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

28.08.2024    1295    Chernazem    3    

6

Рефакторинг и качество кода Программист Бесплатно (free)

SOLID – принципы проектирования программных структур (модулей). Акроним S.O.L.I.D. образован из первой буквы пяти принципов. Эти принципы делают код более гибким, упрощают разработку. Принято считать, что принципы SOLID применимы только в объектно-ориентированном программировании. Но их можно успешно использовать и в 1С. Расскажем о том, как разобраться в принципах SOLID и начать применять их при работе в 1С.

22.08.2024    10751    alex_sayan    41    

53

Рефакторинг и качество кода Программист Стажер Платформа 1С v8.3 Конфигурации 1cv8 Бесплатно (free)

Рассмотрим основные принципы шаблона проектирования "Стратегия" на простом примере.

25.06.2024    4483    MadRave    34    

27

Рефакторинг и качество кода Платформа 1С v8.3 Бесплатно (free)

Поделюсь своим опытом аудита кода авторских продуктов с Infostart.ru как одним из элементов применения DevOps-практик внутри Инфостарт. Будет настоящий код, боевые скриншоты, внутренние мемы от команды ИТ-лаборатории Инфостарт и прочее мясо – все, что любят разработчики.

10.04.2024    13674    artbear    85    

108

Рефакторинг и качество кода Программист Платформа 1С v8.3 Россия Бесплатно (free)

Предлагаю вашему вниманию советы мастеров древности. Программисты прошлого использовали их, чтобы заострить разум тех, кто после них будет поддерживать код. Гуру разработки при найме старательно ищут их применение в тестовых заданиях. Новички иногда используют их ещё лучше, чем матёрые ниндзя. Прочитайте их и решите, кто вы: ниндзя, новичок или, может быть, гуру? (Адаптация статьи "Ниндзя-код" из учебника JavaScript)

01.04.2024    4370    DrAku1a    15    

39

Рефакторинг и качество кода Программист Бесплатно (free)

В новом материале мы анализируем, как в программировании баланс между быстротой разработки и тщательной проработкой кода влияет на конечный продукт. Обсуждаем, почему иногда важнее сосредоточиться на скорости выполнения проекта, и когда можно позволить себе уступить в качестве ради достижения бизнес-целей.

01.04.2024    1359    Prepod2003    6    

2
Комментарии
Подписаться на ответы Инфостарт бот Сортировка: Древо развёрнутое
Свернуть все
1. booksfill 04.06.24 14:31 Сейчас в теме
А как вы боретесь с ненужными циклами code review, ну, чтобы не получалось, что вместо того, чтобы работать будут споры по отдельным замечаниям, особенно, если квалификация разработчика позволяет ему возражать?

Например, "используйте ЗаполнитьЗначенияСвойств", а вам в ответ: "здесь нецелесообразно создавать покрытие тестами и всего 3-и явных присвоения, зато вероятность того, что данный параметр могут переименовать велика, лучше это получить такую ошибку сразу".
Тут холивар не тему покрытия тестами и на то "ну, ежели на то пошло, то параметр могут и не переименовывать, но изменить его тип и помогут тебе твои ляхи с явным присвоением?!" или "а вы знаете, что этот метод далеко не бесплатен, а у нас тут цикл на 1000 000 непойми чего".

Или "НСтр используется даже в базах с одним языком, т.к это сильно облегчит работу переводчику, если понадобится еще одна локализация."
И тут холивар с приведением мнений локализаторов и потерей времени на отстаивание своего мнения, которого до этого могло вообще не быть, ну не лезли оба в тонкости локализации.

Не суть важно, просто мне хотелось бы услышать ваше мнение на тему типа: для того, что вас категорически не устраивает, следует использовать императивы: никаких "возможно, вероятно, кажется", а "Заменить на ЗаполнитьЗначения", "убрать НСтр" и т.п.

И отвечать за них самостоятельно, а не "вместе с тем парнем".

А все остальные, относимое к категории "кажется", "возможно", "может быть" оставить при себе.

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

А ведь есть и еще нюанс, тоже любопытный - разработчик поправил в легаси из 5000 строк десяток, а ему радостно вываливают 100 ошибок, вообще к нему не относящихся. Или вынужденных, ну идет заполнение списка значений поиском по коду, нехорошо, конечно, но сделать добавление еще 1 позиции из 50 "правильно", как-то не правильно.

Словом, не критики ради, но раз уж зашел разговор про такие материи, хотелось бы почитать про нюансы.
Уверен, что "их есть у вас".
Skif1989; manlak; begemot; zqzq; cdrw3; user1067792; SP2000; +7 Ответить
6. mrXoxot 3065 04.06.24 15:20 Сейчас в теме
(1) Да, мы сталкиваемся ровно с такими же проблемами и холиварами. Бывает, что переписка по одному замечанию идет дольше, чем его исправление. С этим ничего не поделать, кажется.

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

Если и это не помогает, то просим дополнительное мнение у еще одного разработчика. Таким образом, 2 против 1 будет :)

Про императивы - у нас не очень принято так писать. Обычно стараемся писать аккуратно :)
Вообще, встречал вот такую градацию:
Рекомендуем = Делать только так
Следует выполнить = Лучше сделать именно так
Можно попробовать = Считаю, что этот вариант будет лучше.

Про легаси - тут мы стараемся править что-то и рядом тоже, хоть и не всегда удается. Поэтому если вдруг рядом прилетели какие-то еще замечания и их несложно поправить (а с BSL обычно не сильно сложно поправить), то можно и поправить. Но бывают варианты и когда четко ограничиваем - здесь делаем, а здесь нет.
9. booksfill 04.06.24 16:14 Сейчас в теме
(6)
Спасибо.
Понял.

P.S.
Не могу удержаться, ниже немного словоблудия :)
"Рекомендуем = Делать только так", ага "через сплошную нельзя, а через двойную сплошную совсем нельзя".
Откуда к нам пришел это слащавый стиль:
- яхте "Беда" рекомендуем сменить курс!
- это еще зачем?
- противник не подчинился приказу, выпустить 2-е торпеды!

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

"Заменить на ЗаполнитьЗначенияСвойств" - нормально.

"Рекомендую, ну сколько можно-то, уже третий раз говорю, заменить на ЗаполнитьЗначенияСвойств" - не нормально.

"Рекомендую заменить на ЗаполнитьЗначенияСвойств" - тоже не нормально, вдруг человек учил русский, а не "европейский" и "рекомендую" воспримет именно как рекомендацию, а не требование.
13. mrXoxot 3065 04.06.24 17:25 Сейчас в теме
(9) Да, вполне может быть. У нас в команде пишут по-разному замечания и тон обращения тоже бывает разный.

Но одно дело когда ты ревьюишь код человека из команды и он легко к этому относится. Другое дело, когда проводишь ревью чужого человека.
43. kuzyara 2095 06.06.24 07:12 Сейчас в теме
(13) по поводу статьи. категорически не согласен с таким подходом

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

Ревью нужен чтобы:
а) не падал прод
б) не было костылей и велосипедов

всё остальное суета ради суеты,
поэтому:
- НЕ выполнять работу вместо сонара/апк/итс
- НЕ растягивать более чем на 1 день

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

Гуглить фразу "плохое code review".

Но автор молодец что пишет об этом, поделился своей болью)
48. alex_sayan 53 27.06.24 07:46 Сейчас в теме
(43)
Ревью нужен чтобы:
а) не падал прод
б) не было костылей и велосипедов


Прежде всего кодревью проводят чтобы убедиться, что код понятен другим разработчикам
51. kuzyara 2095 27.06.24 08:10 Сейчас в теме
(48) ох уж эти "понятия") Вот допустим работает сотрудник в команде, трое человек. Ни в какую не хотят делать ревью, им до лампочки понятность кода коллеги. Обосновывают тем что им за это не платят и на это нет времени. Что бы вы сделали?
52. alex_sayan 53 27.06.24 08:52 Сейчас в теме
(51) для начала нужен административный пендель

Обосновывают тем что им за это не платят и на это нет времени


Блеск! На втыкание в непонятный код тратится гораздо больше времени. То есть потратить час на кодревью, сказать "здесь какая-то невнятная х..ня, перепиши по-человечески" у них времени нет. А в будущем потратить лишние часы на попытки понять тот самый невнятный код коллеги (когда будут работать с ним), у них время будет)
7. mrXoxot 3065 04.06.24 15:28 Сейчас в теме
(1) А Вы как с этим справляетесь?
8. booksfill 04.06.24 15:59 Сейчас в теме
(7) А у меня нет серьезного опыта.
На этом можно было бы и остановиться, но, на всякий случай:

Никаких переписок по явным косякам - только хардкор, только созвон, а лучше просто подойти/включить демонстрацию и вместе пройтись по замечаниям. Это на порядок экономит время обеих высоких сторон и серьезно ускоряет принятие фичи, плюс обычно оба что-то выносят и для себя.
Иногда, вообще, рождается новый, лучший , вариант.
Что-то типа парного программирования по устранению проблем.
Я уж не говорю про случай, когда у одного из разработчиков уровень сильно ниже, учить новичка на рубленных фразах, забывая, что это для вас очевидно такое себе ...

Замечания типа "ну мне так не нравится", оставляем при себе - если есть лишнее время, лучше отрефакторить чего-нибудь полезное.
kuzyara; cdrw3; +2 Ответить
14. mrXoxot 3065 04.06.24 17:27 Сейчас в теме
(8) Да, повышение уровня коммуникации - это очень сильный инструмент.
Раньше когда мы в офисе сидели - спокойно просто подходили к друг другу и вполне быстро все удавалось.

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

Когда вместе разбираешь код - становится сильно понятнее и причину почему сделали именно так, и аргументы почему стоит сделать по-другому.
35. gybson 05.06.24 16:31 Сейчас в теме
(1) Составляется соглашение о разработке в котором все принципиальные моменты прописаны. И про запросы в цикле и про доступ через точку и все-все-все.
38. mrXoxot 3065 05.06.24 21:37 Сейчас в теме
(35) А есть пример такого соглашения?
40. gybson 05.06.24 22:59 Сейчас в теме
(38) Берете стандарты разработки от 1С и далее каждое разногласие обсуждаете и вносите вердикт в соглашение. Даже если я дам вам наше соглашение (а я не могу дать, я связан корпоративной этикой), то вы все-равно его не примите сразу как данность. Это все надо обсудить, доказать верность выбранного решения и зафиксировать его.
Если вы руководитель, то можете навязать (и так и надо сделать :))

Как здесь верно заметили, "сонар" сделает всю грязную работу. Человек смотрит код уже на уровне повыше, ближе к архитектуре. Например цикломатическая сложность или когнитивная очень тонкие понятия. Т.е. функцию ПолучитьТЗИсторияХранилища уже упростили, а можно может и больше упростить и это только человек сделает.

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

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

Вы понимаете что вы делаете, понимает почему и зачем, представляете где этому научились и где это описано и так формируете базу для хорошего кода, которая аргументирована и разумна. Анализ кода ни в коем случае не должен быть вкусовщиной, это прямой путь к конфликтам и качества это не даст.
2. SergeyMordvin 1975 04.06.24 14:38 Сейчас в теме
Класс, почитаю обязательно!
3. slavik27 101 04.06.24 15:08 Сейчас в теме
Подскажите а чем не понравилось слово "Получить"? оно ассоциируется с разными словами, типа "получить по шапке"?
TerveRus; smit1c; SP2000; +3 Ответить
4. mrXoxot 3065 04.06.24 15:12 Сейчас в теме
(3) Есть стандарт - https://its.1c.ru/db/v8std#content:647:hdoc
Суть его в том, что слово Получить нам ничего дополнительно не сообщает. То есть функция по умолчанию что-то возвращает, а значит нужно описать, что конкретно она вернула. То есть вместо ПолучитьМассивРолейСПравомДобавления следует использовать ИменаРолейСПравомДобавления
KamranV21; +1 Ответить
5. slavik27 101 04.06.24 15:16 Сейчас в теме
(4) спасибо про стандарт понятно.
10. Lars Ulrich 625 04.06.24 16:48 Сейчас в теме
(4) Со стандартом не поспоришь, конечно, но всякие геттеры|сеттеры вроде предполагают наличие получить/get|установить/set, поэтому может не все так однозначно должно быть.

Ну и опять же стандарт стандартом, а открываем СП, и видим:
- КлиентскоеПриложение.ПолучитьЗаголовок()
- ПолеHTMLДокумента.ПолучитьURL()
- Глобальный контекст.ПолучитьСтруктуруХраненияБазыДанных()
- РегистрРасчетаМенеджер.<Имя регистра расчета>.ПолучитьБазу()
- ХранилищеНастроекМенеджер.<Имя хранилища>.ПолучитьФормуЗагрузки()
- РегистрСведенийМенеджер.<Имя регистра сведений>.ПолучитьПервое()
...
Видимо, тут не "большинство случаев" или до появления стандарта родилось )) просто мысли вслух

Ну и само собой самое сложное в архитектуре - это правильное именование. "Как вы яхту назовете..."
lostcay; cdrw3; +2 Ответить
33. gybson 05.06.24 14:24 Сейчас в теме
(4) ПолучитьПолноеИмяОбъектаМетаданных и ПолучитьСтрокуРодителя проверку прошли же.
Skif1989; smit1c; +2 Ответить
34. mrXoxot 3065 05.06.24 15:39 Сейчас в теме
(33) Да, это было пропущено при ревью :)
36. gybson 05.06.24 16:32 Сейчас в теме
(34) 1/3, так себе результат =)
49. alex_sayan 53 27.06.24 07:51 Сейчас в теме
(4) кстати, такой себе стандарт. Например, в стандартах Java устанавливается ровно обратное: добавлять к множеству методов префиксы get- (получить) и set- (установить). Мотивируется это тем, что читающих код сразу понимает, что цель этого метода только получить некое значение, или только установить некое значение (львиная доля всех действий в коде)
11. m_aster 114 04.06.24 17:00 Сейчас в теме
Знал я одного такого "типа", "специалиста" по СКД, который сервер 1С ронял, не зная элементарных вещей по обработке результата выборки запроса.
Все смеялся, чего, мол у тебя пробелы между равно и значением, чего заглавные буквы в переменных.
Кстати, о заглавных, пример типового шаблона, если не ошибаюсь, "тянется" еще с семерки:

Для каждого <?"Переменная"> Из <?"Коллекция"> Цикл

<?>

КонецЦикла;

Еще до недавнего времени раздражали неимоверно эти плюсы-минусы в комментах(они даже в ЕРП "пробрались", как-то приводил здесь пример из общего модуля вообще без комментов, но зато плюсы), сейчас проще, но все равно, нигде в типовом коде такого не встречал(в Delphi, например, тоже):

Пример из общего модуля "АвансовыйОтчетФормы" БП 3:

#Область ОбработчикиСобытийФормы

Процедура ПриСозданииНаСервере(Форма, Отказ, СтандартнаяОбработка) Экспорт

ОбновлениеИнформационнойБазы.ПроверитьОбъектОбработан(Форма.Объект, Форма);

// СтандартныеПодсистемы.ПодключаемыеКоманды
ПодключаемыеКоманды.ПриСозданииНаСервере(Форма);
// Конец СтандартныеПодсистемы.ПодключаемыеКоманды

// СтандартныеПодсистемы.ВерсионированиеОбъектов
ВерсионированиеОбъектов.ПриСозданииНаСервере(Форма);
// Конец СтандартныеПодсистемы.ВерсионированиеОбъектов

КонецПроцедуры

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

К именам переменных:
"1. Имена переменных следует образовывать от терминов предметной области таким образом, чтобы из имени переменной было понятно ее назначение."
В "Процедура УстановитьОтборВТЧ()" слегка коробит наименование пВерсия.
Если Вы так педантичны в мелочах, делайте тогда не пВерсия, а, например, ВерсияЭлементаИсторииХранилищаДляОтбораСтрок, либо короче ВерсияИсторииДляОтбора, будет понятнее.
1С тоже иногда отступает от собственных стандартов, на мой взгляд, важнее именно функциональность и оптимальность кода с точки зрения быстродействия и безопасности.
TerveRus; +1 Ответить
15. mrXoxot 3065 04.06.24 17:30 Сейчас в теме
(11)
пВерсия


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

Но в типовом коде такое, как правило, не встречается.
16. mrXoxot 3065 04.06.24 17:32 Сейчас в теме
(11)
Где здесь плюсы-минуcы?


Про плюсы и минусы - не очень понял о чем идет речь.

В комментариях к замечаниям использовал +++ лишь для того, чтобы было проще искать такие замечания.
Можно использовать любые метки для подготовки таких замечаний. В случае если Code-review выполняется в сторонних инструментах, то там вообще этого не требуется.
22. m_aster 114 04.06.24 21:15 Сейчас в теме
(16)Многие франчи даже за стандарт выдают такую структуру комментов:

// ++...
...
// --...

Хотя в типовых я такого не встречал, и в стандартах такого тоже нет. Не так давно в ERP такое увидел, об этом написал выше.
24. triviumfan 97 05.06.24 01:21 Сейчас в теме
(22) и что с ним не так?) Общепризнанный и популярный шаблон комментария с открывающимися и закрывающимися префиксами.
28. mrXoxot 3065 05.06.24 07:47 Сейчас в теме
(22) Да, это понятно.

Это обычно используется, чтобы выделить измененные строчки кода, чтобы упростить себе жизнь при обновлениях.

Не уверен, что сейчас такие комментарии полезны при использовании расширений при доработках.
53. Skif1989 02.07.24 18:14 Сейчас в теме
(22)правило (работает не трогай)
17. mrXoxot 3065 04.06.24 17:34 Сейчас в теме
(11)
Для каждого Из Цикл


Про это тоже есть стандарт - https://its.1c.ru/db/v8std/content/441/hdoc
21. m_aster 114 04.06.24 21:11 Сейчас в теме
(17) По Вашей ссылке:
"Неправильно:

конецЕсли, КОНЕЦЕСЛИ, конецесли, Конецесли."

Я об этом написал выше:
"Для каждого <?"Переменная"> Из <?"Коллекция"> Цикл.."

"каждого" с маленькой, в стандартном шаблоне. По идее должно быть:

"Для Каждого <?"Переменная"> Из <?"Коллекция"> Цикл.."
26. mrXoxot 3065 05.06.24 07:41 Сейчас в теме
(21) В стандарте написано - В конструкциях встроенного языка ключевые слова пишутся канонически (как в документации или Синтакс-помощнике).

В синтакс-помощнике вот такой пример:
Синтаксис:
Для Каждого <Имя переменной 1> Из <Имя переменной 2> Цикл
// Операторы
[Прервать;]
// Операторы
[Продолжить;]
// Операторы
КонецЦикла;
39. m_aster 114 05.06.24 22:23 Сейчас в теме
(26)
Прикрепленные файлы:
50. alex_sayan 53 27.06.24 07:56 Сейчас в теме
(11)
1. Имена переменных следует образовывать от терминов предметной области таким образом, чтобы из имени переменной было понятно ее назначение.


Очень спорная рекомендация. Львиная часть кода работает с какой-нибудь низкоуровневой ерундой, не относящейся к предметной области. Например, в предметной области нет никаких HTTP, XML, СКД...
12. m_aster 114 04.06.24 17:07 Сейчас в теме
(10)Да, как вы яхту назовете, так на ней и напишите.
Хорошо, когда то, что предполагается, реально используется.
Если бы не было стандартов, наверное все было бы печальнее. В любом случае, в каждом случае, нужно исходить из реальной ситуации.
Это как в фильме "Паркер":
- "Когда я с кем-то договариваюсь, правила нужно всем соблюдать. Если меня пытаются дурить, в мою жизнь проникает хаос. Кому нравится хаос?"
19. booksfill 04.06.24 18:41 Сейчас в теме
(12)
Если бы не было стандартов, наверное все было бы печальнее


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

Между прочим, никто не запрещает использовать глаголы в именах методов, просто приводятся случаи, когда этого делать не надо. И с этим случаями я согласен.

Кстати.
От геттеров и сеттетров в том же C# не то, чтобы отказались, но вызываются они неявно.
Например string personName = person.Name; Эквивалентно JAVAскому string personName = person.getName();


P.S.
Что бесит, так это навязывание использования CamelCase в именах локальных переменных.

Ведь я же ничего опять не путаю? Ну путаю ведь? Ведь новый стандарт не вышел? :)

Зачем мешать написать пВыбор, вместо Выбор?

Ну или уж требуйте использовать "this" для обращения к переменным класса.

В крайне вольном переводе на язык 1С, как вариант:
Если "Выбор" - реквизит формы, то
Выбор = 15; или нашВыбор = Выбор; вызванные в в методе - это должна быть ошибка, а не неявное создание переменной.
Единственный вариант обращения к реквизиту формы, обязательно через ЭтотОбъект.Выбор.
А если уж переопределение, то никак не неявное а через Перем.

Да, пВыбор - это коcтыль, но пока конфигуратор ничем тут не помогает (не знаю как в EDT), в 99,99% случаев я знаю, что пВыбор - это про локальную переменную. Да и читать такой код намного легче, за доли секунды понятно, что не надо метаться, это где-то в этом методе.
29. zqzq 25 05.06.24 09:25 Сейчас в теме
(19) У меня пВыбор ассоциируется с параметром метода, в Камине вроде такой подход используется.
Аналогично как мВыбор это переменная модуля объекта (это в типовых 1С встречал).

Согласен, что 1С следовало бы запретить неявное обращение к реквизитам/переменным модуля, только через ЭтотОбъект.
18. partizand 137 04.06.24 18:06 Сейчас в теме
У меня сложилось мнение, что на ревью нужно проверять общий подход к решению и какие то явные ошибки. Не стоит проверять правильность алгоритма. Его проверят тесты или аналитик. Проверять отсутствие пробелов можно доверить машине.
А от споров частично должен спасать регламент, т.е. договорённости команды.
20. mrXoxot 3065 04.06.24 19:07 Сейчас в теме
(18) Да, большинство замечаний к коду вполне находит SonarQube, АПК и BSL плагин - это правда.
Но тут все еще зависит от культуры разработки - не все разработчики смотрят на эти замечания, поэтому приходится на них указывать.

В идеале, на ревью должен быть уже готовый код, который отсмотрел статистический анализ кода, прогнаны все тесты и все прекрасно. И ревьюер может просто посмотреть на общий подход и понятность кода. Но, к сожалению, я таких идеальных проектов еще не встречал..
TerveRus; +1 Ответить
23. cheshirshik 70 04.06.24 22:14 Сейчас в теме
(20) то что у вас пройденные тесты и код в порядке ещё не говорит о том, что программа работает как надо. 😹
TerveRus; +1 Ответить
27. mrXoxot 3065 05.06.24 07:43 Сейчас в теме
(23) Никто с этим и не спорит.
После code-review у нас обычно начинается тестирование. Работа над повышением качества - это большая и комплексная работа, а code-review - это один из способов для такой работы. Но одного этого способа может быть не достаточно.
25. siamagic 05.06.24 01:56 Сейчас в теме
(18) А я вообще не понял за что этому мужику платят и кто он по должности - техничка или секретарша? Если это контора с текучкой, то зачем им эта заморочка? Если контора пишет для себя, то зачем нанимает идиотов за которыми нужны правки? Как не крути ерунда полная.
30. SerVer1C 817 05.06.24 11:13 Сейчас в теме
Половину ревью (а то и больше) сможет взять на себя статический анализатор, например, SonarQube.
31. mrXoxot 3065 05.06.24 11:42 Сейчас в теме
(30) Да, обычно так и происходит.
Разработчик получает замечания от анализатора - исправляет их и только после этого передает код на ревью.

В данном примере я взял обработку с GitHub 7летней давности - не уверен, что Сонар тогда пользовался большой популярностью. Да и сейчас не все еще его используют.

Если есть Сонар - то это упрощает жизнь и разработчика, и ревьюера.
32. mrXoxot 3065 05.06.24 11:44 Сейчас в теме
Может быть кто-то хочет поделиться своим кодом для ревью?
Также посмотрю его и подготовлю статью и видео разбор.
37. grumagargler 726 05.06.24 16:44 Сейчас в теме
> Есть стандарт - https://its.1c.ru/db/v8std#content:647:hdoc
"отличный" стандарт, особенно хорошо работает в условиях перегруженного пространства имён, и "помогает" искать методы в контекстной подсказке через точку. А то, что про этот стандарт забыли сказать разработчикам платформы, которые например завезли "ПолучитьДоступныеГолосаСинтеза()" в 25 платформу, говорит о большой его (стандарта) важности.
Прикрепленные файлы:
41. lostcay 10 06.06.24 06:15 Сейчас в теме
По поводу "Для каждого Из ...", тоже постоянно напрягает в типовых конфигурациях))
А что касается НСтр и СтрШаблон, никогда не пользовался, с шаблоном еще могу понять, когда большие тексты, хотя тоже сомнительно, а вот НСтр... не понимаю, это хорошо когда у вас конфигурация для нескольких языков, но просто так оно зачем? Я такое встречал только в международной версии БСП кажется.
А вот ЗаполнитьЗначениеСвойств() работает медленнее, так что на 3-х свойствах проще написать 3 строчки, да и с точки зрения читабельности тоже, так понятнее что и куда летит, но это уже мое субъективное..
На 3-й картинке, где формируется строка ошибки из параметров, в условии тогда лучше не ЗначениеЗаполнено, а ="", а еще лучше ПустаяСтрока() и там начальная строка с пробелом на конце, то есть будет 2 пробела в итоге, не хорошо. А параметры отбора в массив и СтрСоединить с разделителем пробел.

Всегда есть к чему придраться)
42. lostcay 10 06.06.24 06:37 Сейчас в теме
Неужели программистам нечем заняться, кроме как ревьюить код? Чуть более нагруженный и связный код, чем в примере, и это уже удовольствие далеко не на 5 минут.
И самый главный вопрос. А Кому это надо? Большим компаниям с большими проектами? Так надо платить программистам больше чем мешок риса в месяц (хотя бы 2).
Или франчам? Ну никогда не поверю. Видел столько г*вна от топовых франчей, после которых не хочется работать ни за какие деньги, смотреть физически больно. Понаберут студентов...

(Мое мнение, сильно не плюйтесь) - оптимизация нужна тогда и только тогда, когда этого требует бизнес. Работал с другом над одним большим проектом, так там такое месиво, абсолютно не читаемое и недокументированное, боже упаси туда лезть. Только вот что важно, он сделал это в 10 раз быстрее, чем оценил бит и в разы дешевле. Я потом еще полгода за ним подчищал какие-то куски (для собственного душевного спокойствия). С точки зрения бизнеса он свернул гору, а моя возня нафиг никому не сдалась.
Да и кому вы пытаетесь что доказать? Если хотите карму почистить, займитесь чем-то полезным, не знаю, выложите крутую обработку, поотвечайте на форуме или помогите бабушке перейти дорогу.

Сильно задушнил?))
Skif1989; Slava_prog; chernyakai; Sure; smit1c; kuzyara; +6 Ответить
45. mrXoxot 3065 06.06.24 09:13 Сейчас в теме
(42)
Сильно задушнил?))

Нет :)

Ваше мнение понятно - и про требования бизнеса, и про то что никому не нужно ревьюить код.
Главное - чтобы код работал, а там как он выглядит - пофиг.

Но я с этим мнением не согласен :)
54. Skif1989 02.07.24 18:16 Сейчас в теме
(42)все верно.
Сломалось - изучай.(находи ошибки и исправляй)
А ревью это для бездельников. (прошу прощения если кого то обидел)
44. kuzyara 2095 06.06.24 07:44 Сейчас в теме
Ревью кода — это процесс верификации, контроля, процесс, который должен проходить максимально быстро. И примешивать к нему другие процессы (обучение новичков, обмен знаниями, демонстрация крутизны экспертизы и т. д.) просто неуместно. Это как минимум неоправданно дорого и — главное — не работает на данном этапе.
46. Slava_prog 11.06.24 08:56 Сейчас в теме
Я считаю задача ревью кода найти явные проблемы, которые повесят базу или будут давать неправильные результаты вычислений.
В приведенных примерах такого нет.
Код правильный и понятный.

Если это не разработка типового продукта для международного рынка НСтр, только загромождает код.
По поводу "// +++ Возможно, когда ничего не выбрано, стоит показывать весь список без отбора, а не по 0 версии"
тут мне кажется нужно глянуть в ТЗ, если в ТЗ ничего нет я бы попробовал обы варианта и выбрал более удобный для пользователя.

Вообще похоже у ревьювера в команде хорошие программисты и он заскучал :)

Вот я недавно разбирался с тиражным решением и там был код типа
ПапкаНастроек = "C:\";
artbear; Skif1989; kuzyara; TerveRus; mrXoxot; +5 Ответить
47. пользователь 25.06.24 20:23
Сообщение было скрыто модератором.
...
55. artbear 1563 09.07.24 15:51 Сейчас в теме
(0) я вместо конструкций, в которых можно ошибиться и которые слабо читабельны
Если НайденныеСтрокиИзмененныеОбъекты.Количество() = 0 Тогда
Если НайденныеСтрокиИзмененныеОбъекты.Количество() <> 0 Тогда
Если НайденныеСтрокиИзмененныеОбъекты.Количество() > 0 Тогда

использую более прозрачные и читабельные варианты
Если Не ЗначениеЗаполнено(НайденныеСтрокиИзмененныеОбъекты.Количество()) Тогда
Если ЗначениеЗаполнено(НайденныеСтрокиИзмененныеОбъекты.Количество()) Тогда
56. artbear 1563 09.07.24 15:56 Сейчас в теме
(0) плюс за саму статью, код-ревью давно люблю
но жаль, что в статье все-таки больше внимания уделяется больше "стилевым" различиям, чем реальным проблемам в коде

все-таки большую часть стилевых ошибок найдет BSL LS

а ревьювер должен найти проблемные, подозрительные\странные места и сообщить автору кода
вот таких замечаний не хватает в статье.
Оставьте свое сообщение