Народная мудрость.
Одна пара пожелала пожениться и обрести вечное счастье. Я взорвал их машину у церкви сразу после венчания.
One Wish Grant, фильм Трасса 60.
Ещё одна философская заметка про управление, а данном случае качеством, состоит из трёх частей: очень абстрактной, в меру абстрактной, конкретной и отдельного вывода. Содержит критику существующей практики применения линтеров.
Очень абстрактная часть о качестве
Сначала я хочу поговорить о качестве, а точнее об управлении качеством чего бы то ни было, о продукте в самом широком смысле, продукте как результате человеческой деятельности, будь то создание нового (написание кода или картины, проектирование космического корабля), отсечения лишнего (скульптура, фрезеровка, отбор хороших фруктов), или трансформация (перевозка, заморозка, упаковка, изготовление бензина и пластика из газа).
У хорошего продукта есть некие признаки того что он качественный. У разных продуктов это разные признаки. Например хорошие фрукты вкусно пахнут, хорошо выглядят, имеют хороший вкус.
Сейчас я приведу гиперболизированный пример, и позже перейду непосредственно к коду.
Представьте что у нас есть магазин фруктов и есть проблема, наши фрукты стали хуже продаваться, а у конкурента очереди прям. Мы проводим исследование и узнаём что запах возле наших прилавков не нравится посетителям. А запах у лавок конкурента нравится. О мы нашли проблему, индекс удовлетворённости поксетителя запахом! Давайте её решим, есть же аромамаркетинг, просто поставим автоматические установки возле прилавков и получим прекрасный запах яблоневого сада. Сделали. И индекс удовлетворённости покупателя запахом закономерно запахом попёр вверх. Только вот покупателей теперь ещё меньше.
Если взглянуть серьёзно то изначальная проблема могла быть совершенно разной:
- Наши конкуренты продают столь же качественные фрукты, но сделали аромамаркетинг раньше нас и привлекли посетителей именно запахом.
- Наши фрукты хороши, но фрукты конкурента действительно лучше чем наши (сорта, хранение, что угодно)
- Наши фрукты протухли. Они просто сгнили и воняют.
- Протухли фрукты с прошлого года которые стоят за витриной, а мы надеемся что ещё сможем их продать. А они оттуда воняют.
- У конкурента больше ассортимент
- Конкурент более красиво разложил свои фрукты, в целом такие-же как у нас.
- Там тупо дешевле
- Там продавец красивая, а у нас баба Маня вышла на подмену...
Очевидно что только в первом случае нам поможет аромамаркетинг. В некоторых он может помочь, но при этом может замаскировать настоящую проблему, а в третьем либо не возымеет действия, либо вызовет ещё больше отвращения. И ох как часто проблема именно в том, что фрукты протухли.
На самом деле при появлении такой проблемы нужно комплексно анализировать причины и в каждом конкретном случае принимать специфичное для случая решение.
Ещё более гиперболизировано
У вас есть зелёные помидоры, а вы знаете что продаются только красные. Не надо красить помидоры. Это хорошо, что можно ускорить созревание при помощи этилена. И это будет полноценное созревание, а не покраска. Если бы ускорить было нельзя то нужно было бы выкинуть эти помидоры и добыть новые, уже хорошие.
Иначе говоря, если вы не удовлетворены качеством получающегося продукта, то в цепочке его производства есть неполадки и нужно анализировать и изменять процесс, а не красить на выходе.
Ну вы поняли. Если что-то плохо пахнет, духи не помогут.
В меру абстрактная часть о качестве кода
Среди свойств хорошего кода мы найдём (не сортируя по важности):
- выдержанность стиля,
- читаемость,
- производительность,
- расширяемость,
- прозрачность архитектуры и паттернов.
Это достигается в первую очередь при помощи самодисциплины и уровня квалификации разработчиков, а также, когда разработчиков много, при помощи соглашей об оформелнии кода(code style) и соглашений об архитектуре (MVC, MVVM, ECS, тысячи их). Качественный код появился гораздо раньше чем линтеры, какие либо соглашения и архитектурные паттерны.
Большинство правил линтеров являются чисто косметическими и решают задачу повышения читаемости кода за счёт однообразного применения небольших и локальных практик. Длина строк там, наименования переменных, const везде где нет модификации, иногда даже вводятся ограничения на цикломатическую сложность функций. Речь сейчас не о конкретных правилах, а о том что эти правила вцелом косметические, они помогают коду выглядеть лучше. Ключевое слово тут выглядеть.
Когда какой-либо показатель начинает использоваться как цель, он теряет свою ценность как инструмент.
Вольная трактовка закона Гудхарта.
Теперь проведём аналогию с помидорами. Наш недостаточно созрел. Автоматический линтер скажет нам: «Смотри надо вот тут и тут не нужный цвет». Что сделает программист? Очень часто покрасит. И в этом кроется основная идея моей критики линтеров. Сейчас я приведу конкретный пример, а потом сделаем вывод.
Конкретика
PixiJS 2 февраля 2018 года (год назад).
Прилетает пул реквест. Суть в том, что ранее для рисования кривой использовалось константное количество точек, что очевидно не оптимально. Предложено использовать хитрый алгоритм для оценки длины кривой. Алгоритм не rocket science, но точно не очевидный, опубликован в 2013 году и приведён со ссылкой на статью его автора (наблюдаются проблемы с https). Счастье что он вообще сохранился на личной страничке.
Там приведён код на С (16 строк):
float blen(v* p0, v* p1, v* p2)
{
v a,b;
a.x = p0->x - 2*p1->x + p2->x;
a.y = p0->y - 2*p1->y + p2->y;
b.x = 2*p1->x - 2*p0->x;
b.y = 2*p1->y - 2*p0->y;
float A = 4*(a.x*a.x + a.y*a.y);
float B = 4*(a.x*b.x + a.y*b.y);
float C = b.x*b.x + b.y*b.y;
float Sabc = 2*sqrt(A+B+C);
float A_2 = sqrt(A);
float A_32 = 2*A*A_2;
float C_2 = 2*sqrt(C);
float BA = B/A_2;
return ( A_32*Sabc +
A_2*B*(Sabc-C_2) +
(4*C*A-B*B)*log( (2*A_2+BA+Sabc)/(BA+C_2) )
)/(4*A_32);
};
А в пул реквесте прислан следующий код (JS):
/**
* Calculate length of quadratic curve
* @see {@link http://www.malczak.linuxpl.com/blog/quadratic-bezier-curve-length/}
* for the detailed explanation of math behind this.
*
* @private
* @param {number} fromX - x-coordinate of curve start point
* @param {number} fromY - y-coordinate of curve start point
* @param {number} cpX - x-coordinate of curve control point
* @param {number} cpY - y-coordinate of curve control point
* @param {number} toX - x-coordinate of curve end point
* @param {number} toY - y-coordinate of curve end point
* @return {number} Length of quadratic curve
*/
_quadraticCurveLength(fromX, fromY, cpX, cpY, toX, toY)
{
const ax = fromX - ((2.0 * cpX) + toX);
const ay = fromY - ((2.0 * cpY) + toY);
const bx = 2.0 * ((cpX - 2.0) * fromX);
const by = 2.0 * ((cpY - 2.0) * fromY);
const a = 4.0 * ((ax * ax) + (ay * ay));
const b = 4.0 * ((ax * bx) + (ay * by));
const c = (bx * bx) + (by * by);
const s = 2.0 * Math.sqrt(a + b + c);
const a2 = Math.sqrt(a);
const a32 = 2.0 * a * a2;
const c2 = 2.0 * Math.sqrt(c);
const ba = b / a2;
return (
(a32 * s)
+ (a2 * b * (s - c2))
+ (
((4.0 * c * a) - (b * b))
* Math.log(((2.0 * a2) + ba + s) / (ba + c2))
)
)
/ (4.0 * a32);
}
Код оформлен в полном соответствии с настройками линтера. Указаны описания всех параметров, ссылка на изначальный алгоритм, куча констов, в соответствии с требованием линтера «no-mixed-operators»: 1 расставлены скобочки. Даже для производительности апи сделано не объектным, а с отдельными параметрами, так действительно обычно лучше в JS.
Есть одна проблема. Этот код делает полную херню. (Попытка калькировать на русском выражение fucked up, которое вполне используется в западных публикациях для выражения степени проблемы и вроде как уместно).
c:\rep\pixi\pixi.js\src\core\graphics\Graphics.js
258:26 warning Unexpected mix of '-' and '*' no-mixed-operators
258:32 warning Unexpected mix of '-' and '*' no-mixed-operators
259:26 warning Unexpected mix of '-' and '*' no-mixed-operators
259:32 warning Unexpected mix of '-' and '*' no-mixed-operators
260:24 warning Unexpected mix of '*' and '-' no-mixed-operators
260:30 warning Unexpected mix of '*' and '-' no-mixed-operators
260:30 warning Unexpected mix of '-' and '*' no-mixed-operators
260:36 warning Unexpected mix of '-' and '*' no-mixed-operators
261:24 warning Unexpected mix of '*' and '-' no-mixed-operators
261:30 warning Unexpected mix of '*' and '-' no-mixed-operators
261:30 warning Unexpected mix of '-' and '*' no-mixed-operators
261:36 warning Unexpected mix of '-' and '*' no-mixed-operators
Возвращается огромная длина, и на неё выделяется много точек, хорошо, что там было ограничение сверху, по нему оно и работало. Раньше этот режим был отключен по дефолту, но потом включился для всех (из-за другого бага кстати). Фикс уже вмержили кстати. Я не связывался с автором коммита и не спрашивал его, почему он решил расставить скобки, но чувствую что он запустил линтер, файл конфигурации которого уже есть в PixiJS. Это линтер ему сказал, твой код плох, потому что в нём не хватает скобок, добавь скобки. Опция «no-mixed-operators» говорит что вы не имеете права написать
2*2+2*2
потому что это может привести к плохой читаемости. Эту опцию кто-то создал, потом кто-то включил в проект, что говорит о том, что многие люди считают её полезной.
Вывод
Я не хочу сказать что линтеры зло, но вот такое применение их я считаю злом. Мы (в смысле человечество) смогли автоматизировать обнаружение только малой части признаков хорошего кода, в основном это косметика типа расставленных скобок. Линтеры хороши как инструменты анализа качества кода, но как только мы возводим соблюдение требований линтера в рамки обязательного требования мы получаем это самое соблюдение требований. Ничего кроме соблюдения требований мы не приобретаем. Это как поставить камеру на конвер с помидорами и отправлять на покраску все которые недостаточно красные. Пока мы не давали разработчику инструмент оценки качества внешнего вида кода он мог прислать плохой код, и мы могли это увидеть. Теперь плохой код будет лучше замаскирован. Он будет мимикрировать под хороший, потому что все внешние признаки хорошего кода на нём есть. И мы потеряем линтер как инструмент оценки, ведь весь код соответствует. У нас был инструмент оценки, а теперь его нет, зато код со скобочками, правда не там иногда, но это детали. Итого линтеры считаю классным инструментом, но только если соблюдение требований не становится целью.
И да, тут можно сказать что нет тестов, что не нужно копипастить код, что это stackOverflow development, что не вставляйте в проект код который не понимаете. Это всё да. И это и есть признак плохого кода. Но линтер помог сделать его визуально похожим на всё остальное в проекте. Но линтер никогда не проверит, хорошо ли вы поняли то, что написали.
Иначе говоря я считаю что правильное применение линтера это регулярный запуск его на проекте лидом и оценка того как и что идёт. Ну и правила типа больше скобок богу скобок я в принципе считаю вредными. Когда мы видим что кто-то коммитит код плохого качества, то стоит понять почему он это делает и решить эту проблему на более глубоком уровне. Естественно форматировать код руками не нужно, автоформаттеры я всячески приветствую, но до тех пор пока они не трогают смысловую часть кода никак. Если же мы линтером заставим человека привести код к стандартам, то мы по сути покрасим зелёный помидор в красный цвет. И будет ещё сложнее понять что он на самом деле зелёный. Что делать в открытых проектах с кучей разных людей, вопрос сложнее, но и тут можно подумать что делать.
Стоит сказать что моё такое отношение к линтерам сформировалось давно, более трёх лет назад, но я никак не мог найти подходящего примера в практике, когда линтер сыграл злую шутку. И вот я его нашёл. Тот факт, что я искал его столь долго, говорит что проблема не столь масштабна, либо о том, насколько сложно заметить негативный эффект, однако думаю эта заметка будет полезна. Помните, линтер это инструмент, и как любой инструмент его можно применять во вред и во благо, и как с любым инструментом иногда можно порезаться.
Комментарии (21)
vvadzim
15.02.2019 15:09+3Казалось бы, при чем здесь линтер?
Aquahawk Автор
15.02.2019 15:11До того как линтер заставил разработчика напихать в код скобок, этот код был более читаем. А когда разработчика заставили напихать скобок, он и напихал и линтер успокоился.
vvadzim
15.02.2019 15:18+3Линтер — он настраиваемый. Если проблема с кодом после линтинга, это фейл ответственного за настройку линтера, а не баг линтера. Если у кодера нет возможности просигналить, что линтер криво настроен, это фейл тимлида. Ну и так далее.
Пример из проекта под рукой:
"no-mixed-operators": [ "warn", { "groups": [ ["&", "|", "^", "~", "<<", ">>", ">>>"], ["==", "!=", "===", "!==", ">", ">=", "<", "<="], ["&&", "||"], ["in", "instanceof"], ], "allowSamePrecedence": false, }, ],
MIT License.
Пожалуйста.Aquahawk Автор
15.02.2019 15:28Так нет, проект и считает что так нормально. Т.е. они и хотели так расставлять скобки. На линтер то я не наезжаю, он отработал ровно как настроен. Весь смысл поста в том какие-бы мы правила не настроили, включив линтинг в пре мердж степ мы получим полное соблюдение правил во всём проекте, но толку от этого может оказаться не так много как хочется.
vvadzim
15.02.2019 15:32+3Смысл хотеть от инструмента толку, под который он не создавался?
CrocodileRed
17.02.2019 11:51Очень соглашусь. Автор приписывает инструменту качества, которыми он не обладает. Соответственно и выводы несостоятельны.
powerman
15.02.2019 15:36+1Всё это полная чушь.
Линтеры, которые следят почти исключительно за оформлением кода — просто слабые. Их слабость может быть вызвана сложностью/гибкостью языка, который слишком сложно анализировать, но это ничего не меняет — существуют линтеры, по крайней мере в других языках, которые способны на очень многое помимо проверки оформления кода.
Если Вы способны отличить плохой код только по косвенному признаку (плохое оформление), то у меня для Вас печальная новость: делать ревью Вам ещё рановато. Да, требование запускать линтер приводит к тому, что и хороший и плохой код выглядит одинаково… но нет, это не мешает, а помогает при ревью, потому что легче сосредоточиться на том, что и как этот код делает, не отвлекаясь на то, как он оформлен.
И, как Вам уже ответили выше, если следование требованиям линтера приводит к ухудшению читабельности кода, то им не надо следовать — надо или менять настройки линтера, или отключать его (в идеале — только мешающее правило, а не весь линтер) локально для данной строки/блока/функции (обычно это делается комментарием в определённом формате).
vintage
16.02.2019 11:16Линтеры имеют крайне куцые настройки. В итоге пол файла утыкано комментариями для линтера, которые конечно же ни сколько не мешают читать код, в отличие от лишнего пробела рядом со скобочкой.
powerman
16.02.2019 14:21Зависит от качества линтера. Например, в среднем проекте на Go (около 50к строк) у меня директивы для линтера встречаются один раз на 488 строк, в мелком проекте на Perl (около 2к строк) они встречаются один раз на 89 строк. В обоих случаях чтению кода они совершенно точно не мешают.
vintage
16.02.2019 18:57-1Скорее от притязательности разработчика.
Ну конечно не мешает. Любителям линтеров даже лишний пробел мешает читать код. Что уж говорить про целые строки комментариев врезанные по среди логики. А если вам не мешает, так вам и линтер не нужен. Вы видимо обладаете навыком чтения кода игнорируя лишнее.
vvadzim
15.02.2019 15:39Он будет мимикрировать под хороший
Код хороший, когда он читается легко.
Правильное форматирование — только одно из условий, хоть и обязательное.
Отсутствие ошибок линтера — только одно из условий, хоть и обязательное. Без него с ростом кодовой базы в ошибках утонут и перестанут их исправлять. Кстати, в линтере есть опция игнорировать строчку.
В примере выше, раз команда не может прочитать код, то код для команды плохой.
Для этого после фазы линтинга идёт фаза ревью. Не смог ревьюер прочитать — на переделку. Вполне возможно, на переделку нужно отправить не код, а конфиг линтера. Ну либо самого ревьюера.Aquahawk Автор
15.02.2019 15:43Про условия вы правильно говорите. Основной посыл тут скорее в законе Гудхарта. Иначе получается карго культ «давайте форматировать ради форматировать». Как итог всё отформатировано, а что получилось пофиг. Да код должен быть отформатирован. Но форматирование это следствие а не причина хорошего кода. Это примерно как лечить лихорадку прикладыванием льда. Да, иногда так и нужно делать, но не всегда.
vintage
16.02.2019 11:29-2Вполне возможно, на переделку нужно отправить не код, а конфиг линтера.
А вы когда-нибудь пробовали продавить изменения в конфиг линтера не будучи при этом тимлидом?
Ну либо самого ревьюера.
А вы когда-нибудь пробовали изменить представление человека о хороших и плохих практиках не будучи при этом его прямым руководителем?
vvadzim
16.02.2019 11:45+1Ну да, а что?
1) иногда успешно,
2) иногда неуспешно,
3) иногда успешно после достаточно продолжительного промежутка времени для обдумывания аргументов,
4) иногда успешно после достаточно продолжительного промежутка времени и предоставления возможности людям самим наступить на свои грабли,
5) иногда неуспешно после достаточно продолжительного промежутка времени чтобы понять, что человек был прав,
6) иногда неуспешно после достаточно продолжительного промежутка времени чтобы понять, что, оказывается, те же яйца в профиль,
7) один раз уволился по собственному желанию.
С течением старения доля и количество 1) постепенно растёт. Правда, не сказал бы, что старение — адекватная плата за эффект :(
s_suhanov
16.02.2019 23:52Отсутствие ошибок линтера — только одно из условий, хоть и обязательное
Именно. Это лишь необходимое условие. А автор подумал, что оно же и достаточное. :)
Aquahawk Автор
17.02.2019 09:50Статья как раз о том, что как раз не достаточное. Но, к сожалению, многие люди думают что достаточное. Только моя позиция в том что это не условие, а следствие.
wsf
15.02.2019 15:39+3Из всей статьи я только вижу проблему в том, что функция не покрыта тестами. А линтер и скобки тут вообще нипричем.
bkar
15.02.2019 17:211. Там продавец красивая, а у нас баба Маня вышла на подмену...
Макдональдс копнули глубже.
С красивыми девушками болтают больше – следующие покупатели ждут дольше.
С баб Манями болтают дольше – следующие покупатели ждут дольше.
С женщинами вообще…
Мак нанимал в официантки и поварихи только яценосителей!
Зарплаты были чуть пожирнее, но это было так наркотически выгодно, что, естественно, даже запретили законом.
dth_apostle
16.02.2019 12:36+1мне кажется, одной из проблем является то, что автор коммита прогуливал математику в школе.
defusioner
16.02.2019 22:36Есть одна проблема. Этот код делает полную херню.
Спасибо, сделало мой вечер
Aquahawk Автор
Можно, кстати, в комментариях общими усилиями придумать как правильно должна быть оформлена эта функция. У неё маленькая цикломатическая сложность, она сама по себе не большая, но как она должна быть оформлена? PixiJS это надо сказать граф движок, 2D но всё-же, так что это проект в котором много сложного кода, и выкинуть его из проекта не получится.