Как улучшить свой код - заметки по ревизии кода

В этой статье разберем основные неточности, ошибки, которые встречаются по коду в проектах на Falcon Space. 

Основная часть касается кода на SQL, но эти советы и принципы также можно переложить и на другие языки. 

Хранимые процедуры MS SQL

Именование переменных 

Придерживайтесь некоего стандарта на проекте (неважно какой он будет, важно придерживаться единого стандарта). 

Название функции или процедуры - это глагол + объект (SaveFile). Название таблицы - это список сущностей в множественном числе (products). 

Используйте префиксы подсистем в именах, например msg_getFile

Таблицы называйте с префиксом подсистемы и в множественном числе ord_cartItems.

Мы используем для длинных названий lowerCamelCase с префиксом подсистем в начале. 

Не давайте имена связанные со временем - new_topSuppliers (лучше уж тогда дайте версию форме и назовите topSuppliers2).

Имя процедуры не должно вводить в заблуждение. Если процедура идет на чтение (GetItems), то не нужно в ней производить какие-то модификации данной сущности (например, запускаем обновление статуса данной сущности). Другой пример: isDb - это переводится как "Это база данных", а не признак того, что это дашборд.

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

Работа с SELECT

Идентификатор-колонку (id) лучше всегда ставить в самое начало. 

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

Плохой пример, который сложно читать и поддерживать: 

if isnull(@ppassword,'')='' begin select 0 Result,'Пароль не может быть пустым!' Msg return end

Не нужно делать многоэтажные подзапросы, лучше активно используемые данные в нескольких местах запроса получить через inner join

Не используйте функцию dbo.getUserRoles и другие тяжелые функции в where, order by, group by. В некоторых случаях заменяйте на join выборку по роли (иначе медленно работает на больших данных).

Учитывайте, что в подзапросе может быть больше 1 значения (хотя бы top 1 используйте)! 

-- так делать не нужно
where value >= (select startingPrice from ms_auctionItems where id = cast(@itemID as int))  
-- вместо этого вариант лучше использовать in
where userID = (select id from as_users where username=@username)  

Взять за правило именовать таблицу кратко (особенно при JOIN) и обращаться к ней при обращении к столбцам (это уменьшит вероятность коллизии при правке запроса и при добавлении новых столбцов в структуру БД): 

-- плохо
select name from products

--хорошо
select p.name from products p

Нет смысла использовать в where isnull (orderID, 0) = 0 , когда orderID это внешний ключ, orderID не может быть равен 0:

select code from as_coupons where isnull(orderID,0)<>0)

-- можно просто orderID is not null

Когда делаете конкатенацию строк для вывода - обязательно для отдельных параметров проверяем isnull (иначе есть риск, что вся строка будет NULL)

select isnull(x1, '') + ' ' + isnull(x2, '')

Оптимизация. Предпочитайте Exists вместо in, поскольку при применении EXISTS не происходит выборка строк, то его использование более оптимально и эффективно, чем использование оператора IN.

Безопасность 

Обязательно проверяйте, что у компонентов установлены поля Роли - это базово отсечет по роли доступ к данному компоненту

При проверке доступа опирайтесь только на @username, именно этот параметр в процедурах определяет кто именно сейчас работает с системой. Ни в коем случае не опирайтесь на входные бизнес-данные при проверке доступа (например, на orderID). Злоумышленник может изменить запрос вручную и передать чужой orderID. 

Нельзя просто делать исходя из того, что только какой-то роли доступен компонент: 

delete from as_cat_productAttrs where productID = @itemID

Человек может подставить любой itemID и таким образом удалить чужие данные!

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

Если используете внешний iframe, то для повышения безопасности используйте этот атрибут - http://htmlbook.ru/html/iframe/sandbox

Строки 

Если часть строки будет null, то вся строка будет null. Поэтому используйте при конкатенации строк в одну для отдельных переменных isnull.

При приведении типов из строки используйте try_cast, try_convert вместо cast и convert. Этим вы избежите исключения (функция вернет null если нельзя привести тип к заданному).

При конкатенации строк таблицы в 1 строку с помощью For XML обязательно используйте NVARCHAR а не VARCHAR (иначе некоторые символы становятся нечитаемые). Если у вас некоторые символы выглядят как "?", то вероятно вы используете varchar (не поддерживает юникод) вместо NVARCHAR (поддерживает юникод). Проверьте типы данных для переменных, для столбцов, а также наличие знака N для строковых констант

select N'Строка с символами в юникод'

Структура кода процедур и функций 

В функциях @res обозначает выходную переменную и она всегда возвращается в конце функции.

Используйте try catch для обработки непредвиденных ситуаций (например, выдавая некий типовой результат ошибки в SELECT 1).

Используйте iif вместо case  (это более читаемо и выглядит компактнее в запросах select).

Подписывайте выходные SELECT такими комментариями (так вы не запутаетесь с порядком выходных select)

-- SELECT 2
select 1 Result, '' Msg

При написании кода сразу ставьте пары begin end. Этим снижается риск забыть закрыть парные скобки.

Для IF, While всегда используйте begin end - это упрощает чтение и снижает риски внесения ошибок при изменении кода. 

Соблюдайте отступы, отражающие иерархию кода и запросов (особенно в случае сложных подзапросов в where или select).

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

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

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

insert/ update лучше сопровождать комментариями, что делаем по бизнес-логике (Обновляем текущего сотрудника)

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

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

Убирайте неактуальные комментарии (которые не добавляют понятности процедуре).

Очень длинные строки разбивайте на части (чтобы не нужно было прокручивать вправо).

Разное

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

select @statusDoneID = id from fin_financeStatuses where name = 'Проведен' -- плохо

select @statusDoneID = id from fin_financeStatuses where code= 'done' -- хорошо

Стараемся не использовать магические числа (в крайнем случае их пояснять через комменты). 

Трассировку значений делайте через execute as_print '123', а не через insert into as_trace(...). Данные print выводятся на странице /start

Аккуратнее делайте приведение строки к строке (помните о размере)

declare @s nvarchar(max) = 'dae52ef9-12b1-4d58-97b3-54bc8e018f9f'
select try_cast(@s as nvarchar) -- НЕВЕРНО! dae52ef9-12b1-4d58-97b3-54bc8e
select try_cast(@s as nvarchar(200)) -- ВЕРНО dae52ef9-12b1-4d58-97b3-54bc8e018f9f

Используйте эти комментарии для типовых ситуаций: 

--IMPORTANT важное изменение в работающем проекте (на что обратить надо внимание либо временное решение)
--TODO - т.е. надо доделать
--OPT - надо оптимизировать в будущем (потенциальная утечка)

При работе с большими таблицами имеет смысл ставить некий ограничитель на извлечение данных (например, top 500). Иначе есть риск извлечения очень большого количества данных на страницу, что создаст лишнюю нагрузку на систему. 

Во вложенных процедурах не должно быть дополнительных выходных select, иначе это повлияет на обработку результатов основной процедуры (очередность выходных SELECT для компонента). Если нужны данные из вложенной процедуры, то можно их получать либо через параметр процедуры OUTPUT, либо через insert into table execute SP.

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

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

insert into as_codeRevisions(sp, text, created, createdBy )
select ...

Работа с компонентами платформы

Работа с формами

Не нужно помещать в data-itemID совершенно разные сущности (например, для Роли 1 там передается id товара, а для Роли 2 передается id заказа). 

Не нужно скрывать ненужную форму через класс hide или d-none. Нужно убирать ее из разметки если она не нужна - ведь она все равно загружается и занимает ресурсы

<div class="as-form mt-3 d-none" data-code="topSuppliers" data-itemid="0"></div> 

В форме в SaveItem используйте универсальный параметр @parameters, а не старый способ с @field полями (это удобнее для обработки, особенно в случае добавления новых полей).

В общем случае предпочитайте локальную переинициализацию в каком-то контейнере через '.pHtml' RefreshContainer, нежели полную перезагрузку страницы (через SuccessURL).

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

-- Form GetItem SELECT 2
select  1 DisableFocusOnLoad

Если у вас нет таблиц/форм на странице, то не будут работать динамически добавленные элементы as-form-modal, as-table-modal
Необходимо неявно подгрузить скрипты таблицы/форм через расположение элементов на странице:

<a href="#" class="as-form-modal hide">For modal in search</a>
<a href="#" class="as-table-modal hide">For modal in search</a>

Избегайте активного использования dependmentCols (элементы пересоздаются при изменении родителя) и changeField. В случае возможных коллизий попробуйте менять timeout для checkFiled форм as.option("form.checkFieldDelay", 1500)

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

Работа с таблицами

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

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

В таблицах не забывайте ставить сортировку по умолчанию (когда пользователь зашел первый раз у него @sort не установлен). Пример:

...
order by
case when (@sort = 'created' and @direction = 'down') or isnull(@sort,'')='' then createdForSort end desc,

Если вы планируете использовать сокращенные ссылки типа products/12, а не /product?itemID=12, то используйте передачу параметра в снипете data-itemID='kak-uluchshit-svoy-kod---zametki-po-revizii-koda', не настройку у таблицы "Добавка к адресу URL" (в этом случае он не распознает itemID в строке /product/12).

В Getitems таблиц нельзя менять pageSize

В целом по компонентам

В общем случае старайтесь избегать суперуниверсальных форм/таблиц на 2+роли. В итоге это сложнее поддерживать, есть риски по безопасности (случайно забыть про обработку ситуации с какой-то ролью), сложнее тестировать (внесли изменения для одной роли, а тестировать теперь надо под всеми ролями). Также есть большой риск, что в будущем формы будут все больше и больше отличаться для разных ролей (и увязывать это между собой также будет все сложнее).  

• Когда использовать отдельные формы/таблицы: роли сильно разные, цели использования сильно отличаются, развиваться скорее всего будут в разную сторону (например, Форма заказа для Поставщика и Покупателя).
• Когда использовать одну форму для нескольких ролей: когда цель использования формы одинаковая, роли представляют одну сторону (например, форма модерации для Модератора и Админа).

Не смешивайте крупные блоки HTML кода и SQL. Простую верстку можно оставить в SQL, а для многострочных блоков лучше использовать html блоки (через процедуры as_block, as_htmlBlock).

Если много данных предполагается выводить на странице (например, у нас 4 000 категорий), то обязательно используйте компонент Таблица с пагинацией (плохой пример - дерево из 6000 элементов выгружается в компонент Дерево).

Придерживайтесь структуры процедур для компонентов. Не нужно их кардинально изменять по структуре, т.к. это усложнит поддержку такой процедуры.

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

Для уведомлений - не нужно в additional писать повтор того же, что и в text (лучше тогда оставить пустым additional. Иначе это заставляет пользователя смотреть детали, а там тот же текст, что он уже видел).

Если проект подразумевает в будущем локализацию, то сразу используйте для всех меток в sql функцию dbo.as_lang

select  dbo.as_lang('createSetting', 'Create setting', 'tbSettings', @langID)

Если у вас возникают подобные ошибки при выполнении Внешних действий, то скорее всего вы неправильно передаете выходные типы данных ((например, строка вместо 0)) или параметры имеют имя в неверном регистре (например Code вместо code): has some invalid arguments exception, Error parsing column 0

Работа со страницами

В большинстве случаев вместо /order?itemID=12 можно писать адрес /order/12. Адрес всегда начинается со слеша!  

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

Верстка и работа с HTML

Cледите за иерархией заголовков и структурой страниц, направленных на продвижение. У страницы должен быть 1 тег h1 и остальные теги h2-h6 должны определять скелет документа и иерархию контента на странице (например, не нужно делать в разделе с заголовком h3 подраздел с h2). 

Указывайте все классы адаптивности bootstrap, а не только col-md-9

<div class="col-md-9"></div> - плохо
<div class="col-12 col-sm-7 col-md-9 col-lg-10"></div> - хорошо

Не используйте класс col-xs-12 (вместо него надо col-12). Общая сумма в row должна быть 12 (а не 10). 

Не используйте картинки и другие ресурсы, как пути к совершенно другому сайту. Ссылайтесь только на ресурсы, которые принадлежат вашему проекту. Это снижает зависимость проекта от внешних изменений. 

Адреса к локальным ресурсам, страницам должны начинаться всегда со слеша. Например: /product

<div class="as-form mt-3 d-none" data-code="topSuppliers" data-itemid="0"></div> 

Не создавайте левые кастомные стили (для кнопок и др.). Используйте только стандартные bootstrap кнопки. По максимуму используйте стандартные стили bootstrap. CSS используйте только для дополнительного позиционирования. А стилизацию лучше выполнять за счет генерации новой темы сайта. 

Никогда не используйте inline стили прямо в разметке. По максимуму задействуйте классы Bootstrap. Если нужно еще что-то стилизовать через CSS, то вешаем на элемент уникальный класс (с префиксом по подсистеме) и в CSS страницы или глобальном CSS прописываем стиль для этого класса.

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

CSS стили не надо в форму вставлять (в кастом разметку формы вставлять теги style). Вставляйте их в страницу в поле кода для CSS.

На странице должен быть 1 h1. Соблюдайте иерархию заголовков (h2-h6 определяют структуру документа).

Используйте для кнопок в разметке тег а, а не button (с button замечена проблема анимации появления блоков при enableAnimation=1).

См. также Соблюдение порядка в проекте в процессе сопровождения сайта.