По следам код-ревью

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

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

Ранее (очень давно) я уже писал обзор про часто встречающиеся ситуации (Типичные ошибки, некоторые вопросы качества и эффективности работы при разработке в 1С). 

1. Используйте в команде код-ревью (обзор кода). 

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

Если Вы "созрели" для проведения код ревью, то дерзайте. Некоторые аргументы для нашей команды:

1. Это мощная проверка того что делает джун или новичок. 
2. Позволяет отсечь ошибки
Хочу заметить, тут нет цели найти все 100% ошибок (это вопрос тестирования), нашли несколько уже здорово. 
3. Человек, который знает что его будут проверять, уже пишет лучше. 
У нас есть инструкция для код-ревьювера на что он обязательно должен обращать внимание и если эти пункты нарушаются, то задача идет на возврат. 
4. Все мы повышаем свою квалификацию. 
После многократного разбора ошибок и методик "казусов" стало значительно меньше. 
5. Уменьшается эффект bus factor (эффект автобуса) 
6. Позволяет выбрать наиболее лучшее решение. 
Проверяющий (щие) может не согласиться с реализацией и тогда проблема эскалируется и формируется коллегиальное обсуждение.

2. Используем «Связи параметров выбора» и «Параметры выбора»

для свойств реквизитов и свойств полей формы. Это в большинстве случаев делается легко и быстро особенно при доработках типовых конфигураций.

Если не делать никакого отбора, то получим вывод всех значений и возможность выбора даже не допустимых комбинаций. В качестве примера рассмотрим ограничение выбора для ставки НДС из типовой конфигурации.

И далее в модуле выбора мы видим код по обработке данной ситуации:

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

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

Замечу, что подобный подход не защищает от ошибки при программном создании.

2. Следуем общему стилю кода.

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

2.1. Форматирование. Это легко – выделите необходимый текст и нажмите «Shift+Alt+F». 

2.2. Объявления названий реквизитов, общих модулей, объектов:

В типовых конфигурациях принято использовать параметр определяющий уникальность строки документа как «Код Строки». Но вот такая доработка "слегка" рушит общий подход.

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

Представим, что у нас есть новый документ с таблицей «Товары» и реквизитом «Код Строки Документа» в ней и к нам на вход приходит запрос (для примера переименовывать колонку не будем, можем забыть или результат типового запроса, который не меняем). И далее мы хотим заполнить наш документ.

Не забываем, что 1С рекомендует использовать процедуру «ЗаполнитьЗначенияСвойств» - это самый оптимальный вариант заполнения данных. И вместо вот этого кода:

Пока Выборка.Следующий() Цикл
  ЗаполнитьЗначенияСвойств(Объект.Товары.Добавить(),Выборка) 
КонецЦикла;

Нам придется делать так:

Пока Выборка.Следующий() Цикл
  стр_н = Объект.Товары.Добавить(); 
  ЗаполнитьЗначенияСвойств(стр_н,Выборка)
  стр_н.КодСтрокиДокумента = Выборка.КодСтроки; 
КонецЦикла;

2.3. Использование типовых подходов. Сформировать новые движения в регистр документа можно так (через конструктора):

Процедура ОбработкаПроведения(Отказ, РежимПроведения)
...
    // регистр ЗаказыКлиентов Расход
    Движения.ЗаказыКлиентов.Записывать = Истина;
    Для Каждого ТекСтрокаТовары Из Товары Цикл
        Движение = Движения.ЗаказыКлиентов.Добавить();
        Движение.ВидДвижения = ВидДвиженияНакопления.Расход;
        Движение.Период = Дата;
        Движение.ЗаказКлиента = ТекСтрокаТовары.ЗаказКлиента;
        Движение.Номенклатура = ТекСтрокаТовары.Номенклатура;
        Движение.Характеристика = ТекСтрокаТовары.Характеристика;
        Движение.КодСтроки = ТекСтрокаТовары.КодСтроки;
        Движение.Склад = ТекСтрокаТовары.Склад;
        Движение.Серия = ТекСтрокаТовары.Серия;
        Движение.Заказано = ТекСтрокаТовары.Количество;
        Движение.КОформлению = ТекСтрокаТовары.Количество;
        Движение.Сумма = ТекСтрокаТовары.Сумма;
    КонецЦикла;
...
КонецПроцедуры

Но лучше использовать типовой подход "Проведения документов" при выполнении процедуры проведения:

Процедура ОбработкаПроведения(Отказ, РежимПроведения)
...
    Документы.РеализацияТоваровУслуг.ИнициализироватьДанныеДокумента(Ссылка, ДополнительныеСвойства); // тут мы инициализируем таблицы для регистров
...
    ЗаказыСервер.ОтразитьЗаказыКлиентов(ДополнительныеСвойства, Движения, Отказ); // так выполняем запись
...
    ПроведениеСерверУТ.ВыполнитьКонтрольРезультатовПроведения(ЭтотОбъект, Отказ); // тут выполняется контроль
...
КонецПроцедуры

3. Используем «запросную модель», а не «объектную модель».

К примеру, у вас стоит задача проверить наличие признака «Налогообложение НДС» заказа клиента, и так делать не хорошо:

Если ЗаказКлиента.НалогообложениеНДС=ПредопределенноеЗначение("Перечисление.ТипыНалогообложенияНДС.ПродажаНаЭкспорт") Тогда
    // делаем что-то
КонецЕсли;

Первая проблема, с которой вы столкнетесь, то это снижение быстродействия, иногда существенное. А это все потому, что платформа 1С в данном случае получает весь объект целиком.
А если в документе существует реквизит с типом «Хранилище значения» и в нем хранится скан сертификата в 10 МБ? 

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

Лучше делать так:

мНалогообложениеНДС = ОбщегоНазначения.ЗначениеРеквизитаОбъекта(ЗаказКлиента,”НалогообложениеНДС”); 

Если мНалогообложениеНДС=ПредопределенноеЗначение("Перечисление.ТипыНалогообложенияНДС.ПродажаНаЭкспорт") Тогда
       // делаем что-то
КонецЕсли;

Если проверять необходимо, даже при отсутствии прав, то не забываем использовать процедуру "УстановитьПривилегированныйРежим";

Объектная модель с остатками:

…
мОтбор = новый Структура();
мОтбор.Вставить("Склад",Склад);
// получим остатки
ТаблицаОстатков = РегистрыНакопления.ТоварыНаСкладах.Остатки(ТекущаяДата(),мОтбор,"Склад,Номенклатура","ВНаличии");
…

Так переписать код лучше:

Запрос = новый Запрос();
Запрос.Текст = "ВЫБРАТЬ
    |    Ост.Склад КАК Склад,
    |    Ост.Номенклатура КАК Номенклатура,
    |    Ост.ВНаличииОстаток КАК ВНаличииОстаток
    |ИЗ
    |    РегистрНакопления.ТоварыНаСкладах.Остатки(, Склад = &Склад) КАК Ост";
Запрос.УстановитьПараметр("Склад",Склад);
ТаблицаОстатков = Запрос.Выполнить().Выгрузить();

4. Используем возможности БСП.

В качестве примера рассмотрим программное задание отборов для динамических списков. (Не стоит изобретать велосипед, т.к. для большинства задач можно с успехом применять скрытые возможности БСП).

// Ищем группу
МассивГруппаИЛИ = ОбщегоНазначенияКлиентСервер.НайтиЭлементыИГруппыОтбора(Список.Отбор,,"ГруппаИЛИ");

// Добавляем если не найдена
Если МассивГруппаИЛИ.Количество()=0 Тогда

   ГруппаИЛИ = ОбщегоНазначенияКлиентСервер.СоздатьГруппуЭлементовОтбора(СписокПередачВозвратов.Отбор.Элементы, "ГруппаИЛИ", ТипГруппыЭлементовОтбораКомпоновкиДанных.ГруппаИли);
   ГруппаИ1 = ОбщегоНазначенияКлиентСервер.СоздатьГруппуЭлементовОтбора(ГруппаИЛИ, "ГруппаИ1", ТипГруппыЭлементовОтбораКомпоновкиДанных.ГруппаИ);
   ГруппаИ2 = ОбщегоНазначенияКлиентСервер.СоздатьГруппуЭлементовОтбора(ГруппаИЛИ, "ГруппаИ2", ТипГруппыЭлементовОтбораКомпоновкиДанных.ГруппаИ);
   ОбщегоНазначенияКлиентСервер.УстановитьЭлементОтбора(ГруппаИ1,"Организация",ВладелецТовара,ВидСравненияКомпоновкиДанных.Равно);
   ОбщегоНазначенияКлиентСервер.УстановитьЭлементОтбора(ГруппаИ2," Склад", Склад,ВидСравненияКомпоновкиДанных.Равно);

КонецЕсли;

// добавим к корню отбор
ОбщегоНазначенияКлиентСервер.УстановитьЭлементОтбораДинамическогоСписка(СписокПередачВозвратов,"Склад",Склад,ВидСравненияКомпоновкиДанных.Равно);

5. Используйте свойство «Пользовательская видимость» 

для элементов формы.

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

К примеру, создаете реквизит для себя, снимаете пользовательскую видимость как на рис. ниже. 

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

5.2. Для форм выбора и списков. Обычно при создании форм конструктором форм ссылка не передается на форму и при выборе или выводе вы будете ограничены тем списком, который изначально выведен.

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

6. Вывод первого поля табличной части документа в список. 

Если вам требуется вывести в списках какое-либо поле для дополнительной информации из табличной части, к примеру, первую номенклатуру в документе «Этап производства» или первого партнера в «Задание на перевозку».

Вот так делать не стоит:

Запрос = новый Запрос();
Запрос.Текст = "ВЫБРАТЬ
|	ЭтапП.Ссылка КАК Ссылка,
|	ЭтапП.Номер КАК Номер,
|	ЭтапП.Дата КАК Дата,
|	ЭтапП.Статус КАК Статус,
|	ЕСТЬNULL(ВыхИзд.Номенклатура, ЗНАЧЕНИЕ(Справочник.Номенклатура.ПустаяСсылка)) КАК Номенклатура
|ИЗ
|	Документ.ЭтапПроизводства2_2 КАК ЭтапП
|		ЛЕВОЕ СОЕДИНЕНИЕ Документ.ЭтапПроизводства2_2.ВыходныеИзделия КАК ВыхИзд
|		ПО (ВыхИзд.Ссылка = ЭтапП.Ссылка)
|			И (ВыхИзд.НомерСтроки = 1)";

Оптимальным вариантом будет добавить в шапку документа новый реквизит и заполнять его при записи. Благодаря такому подходу вы с высокой эффективностью решите вопросы вывода дополнительной информации без существенного снижения производительности.

Код получится для запроса следующим:

Запрос = новый Запрос();
Запрос.Текст = "ВЫБРАТЬ
|	ЭтапП.Ссылка КАК Ссылка,
|	ЭтапП.Номер КАК Номер,
|	ЭтапП.Дата КАК Дата,
|	ЭтапП.Статус КАК Статус,
|	ЭтапП.НоменклатураПервойСтроки КАК Номенклатура // лучше реквизит назвать 'мм_НоменклатураПервойСтроки' 
|ИЗ
|	Документ.ЭтапПроизводства2_2 КАК ЭтапП";

7. Выбор и вывод дополнительных данных от полей составного типа

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

При решении подобной задачи удобно создать промежуточный регистр сведений (пример из типовых конфигураций - «Реестр Документов»). В этих регистрах хранится дополнительная информация «Номер документа», «Склад», «Организация», «Статус» и др. нужные информационные поля и дополнительно ссылка на документ или справочник (для связи). 
В результате вместо тяжелого запроса RLS для составного типа будет использоваться только один простой. 

8. Выполнение сложных вычислений в динамических списках/запросах.

Если требуется для каждого объекта в списке вывести сложно/трудоемко (или вообще не возможно запросом) вычисляемую информацию, к примеру, процент оплаты по заказам или состояние оформления документов, то выполнять эти вычисления необходимо отдельно, а не в запросе. Результат этих вычислений удобно хранить в промежуточном регистре сведений.

Примеры из типовой конфигурации - «Состояния заказов клиентов», «Состояние Отгрузки», «Состояния ЭД» и т.д.

Первоначально добавляется регистр с необходимыми полями и ссылкой на справочник или документ. Для рассматриваемого примера "механизм состояний заказов клиентов" существует регистр сведений «Состояния заказов клиентов»:

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

Для документа Заказ Клиента эта процедура выглядит следующим образом (не забываем добавить в другие документы - поступления денежных средств и т.д.):

Процедура ОбработкаПроведения(Отказ, РежимПроведения)
   …
   РегистрыСведений.СостоянияЗаказовКлиентов.ОтразитьСостояниеЗаказа(Ссылка, Отказ);
   …
КонецПроцедуры

В результате запрос динамического списка получился простым, а состав выводимой информации "сложный":

9. Магическое число «семь плюс-минус два»

или куча переменных в параметрах функции. Пусть у вас есть процедура "СделатьЧтоТо", и Вы ее создаете заново или дорабатываете и получаете такую замечательную запись в итоге:

Процедура СделатьЧтоТо(Организация, Партнер, Контрагент, Соглашение, ДоговорКонтрагента, СуммаДокумента, НалогообложениеНДС и т.д. )
   ...
КонецПроцедуры

Стоп! Это перебор, необходимо остановиться и сократить количество параметров функции. Вынесите их в одну переменную с типом "Структура":

Процедура СделатьЧтоТо(Параметры) // Параметры - это структура с передаваемыми реквизитами.
  Организация = Параметры.Организация;
  Партнер     = Параметры.Партнер;
  ...
КонецПроцедуры

10. Излишняя сложность 

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

Пусть у нас есть некая функция "СделатьЧтото(Параметр булево)", которая выглядит следующим образом:

Процедура СделатьЧтото(Параметр булево)
    Если Параметр=Истина Тогда
       // … что-то 1
       // к примеру, свернем таблицу товаров
       Товары.Свернуть("Номенклатура,Характеристика","Количество,КолчиествоУпаковок"); 
    Иначе
       // … что-то 2
       // к примеру, скроем видимость элементов на форме
       Элементы.Группа.Видимость = Ложь;
    КонецЕсли;
КонецПроцедуры

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

&НаСервере
Процедура ПриСозданииНаСервере(Отказ, СтандартнаяОбработка)
   …
   СделатьЧтото(Ложь);
   …
КонецПроцедуры


&НаСервере
Процедура ПередЗакрытиемНаСервере()
   …
   СделатьЧтото(Истина);
   …
КонецПроцедуры

Лучше всего такой код переделать на отдельные функции:

&НаСервере
Процедура ПриСозданииНаСервере(Отказ, СтандартнаяОбработка)
   …
   СкрытьЛишниеЭлементыНаФорме();
   …
КонецПроцедуры


&НаСервере
Процедура ПередЗакрытиемНаСервере()
   …
   ОбработатьТаблицуТовары();
   …
КонецПроцедуры


#Область ДополнительныеПроцедуры

&НаСервере
Процедура СкрытьЛишниеЭлементыНаФорме()
   // к примеру, скроем видимость элементов на форме
   Элементы.Группа.Видимость = Ложь;
КонецПроцедуры

&НаСервере
Процедура ОбработатьТаблицуТовары()
   // к примеру, свернем таблицу товаров
   Товары.Свернуть("Номенклатура,Характеристика","Количество,КолчиествоУпаковок"); 
КонецПроцедуры

#КонецОбласти

11. Дублирование кода.

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

В качестве примера рассмотрим следующую задачу: Первая часть. Вам необходимо добавить на некоторую форму реквизит типа булево с наименованием "Скрыть видимость дополнительных данных". При изменении на значение "истина" будет скрываться видимость группы "Групповая детализация". Делаем реализацию форма:

И код обработчика "при изменении":

&НаКлиенте
Процедура СкрытьВидимостьДополнительныхДанныхПриИзменении(Элемент)
	Элементы.ГруппаДетализация.Видимость = НЕ СкрытьВидимостьДополнительныхДанных;
КонецПроцедуры

Вторая часть задачи: Заказчик попросил сделать реквизит "Скрыть видимость дополнительных данных" сохраняемым на форме.

Мы переключаем свойство "Автоматическое сохранение данных" в "Использовать" и теперь, при открытии формы нам нужно изменить видимость. И сразу хочется написать следующий код:

&НаКлиенте
Процедура ПриОткрытии(Отказ)
	Элементы.ГруппаДетализация.Видимость = НЕ СкрытьВидимостьДополнительныхДанных;
КонецПроцедуры

Стоп. Так делать нельзя! В этот момент вы должны выделить код первой функции и нажать правую кнопку мыши и выбрать команду «рефакторинг». И вынести в отдельную процедуру. (Лениво, но сделать надо)

В итоге должно получиться так:

&НаКлиенте
Процедура СкрытьВидимостьДополнительныхДанныхПриИзменении(Элемент)
	СкрытьВидимостьДополнительныхДанных(); 
КонецПроцедуры


&НаКлиенте
Процедура ПриОткрытии(Отказ)
	СкрытьВидимостьДополнительныхДанных();
КонецПроцедуры

&НаКлиенте
Процедура СкрытьВидимостьДополнительныхДанных()	
	Элементы.ГруппаДетализация.Видимость = НЕ СкрытьВидимостьДополнительныхДанных;
КонецПроцедуры

Третья часть задачи: Добавили еще одну вкладку "Супер дополнительные данные" и ее надо также скрывать при изменении реквизита.

&НаКлиенте
Процедура СкрытьВидимостьДополнительныхДанныхПриИзменении(Элемент)
	СкрытьВидимостьДополнительныхДанных(); 
КонецПроцедуры

&НаКлиенте
Процедура ПриОткрытии(Отказ)
	СкрытьВидимостьДополнительныхДанных();
КонецПроцедуры

&НаКлиенте
Процедура СкрытьВидимостьДополнительныхДанных()	
	Элементы.ГруппаДетализация.Видимость       = НЕ СкрытьВидимостьДополнительныхДанных;
	Элементы.ГруппаСуперДетализация.Видимость  = НЕ СкрытьВидимостьДополнительныхДанных;
КонецПроцедуры

Необходимо выработать правило на уровне ощущений, если чувствуете, что код повторяется, то надо остановиться и вынести дублирование в отдельную функцию.

Update: В статье Как завести у себя в команде код-ревью. Отвечаем на вопросы - отвечаем на организационные, технические и процессные вопросы. Статья построена в формате вопрос-ответ.