Ревью кода: почему это нужно веб-разработчикам

Код работает, проходит автоматические проверки и соответствует правилам команды. Что ещё должен увидеть человек, которому мы отправляем его на ревью? И как понять, что его замечания помогут проекту, а не заставят переписать всё под чужой вкус?

Мы с наставником HTML Academy Лизой Пак разобрали эти вопросы на работах участников марафона по вёрстке. У всех был один макет, но решения получились разными. Где-то лишняя обёртка ничего не меняла, где-то недостающая мешала собрать слайдер. А за аккуратным совпадением с картинкой могли скрываться проблемы, которые проявятся после первой замены текста.

Зачем смотреть на работающий код

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

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

Я попросил Лизу сравнить её опыт в продуктовой компании и в студии. К моменту нашего разговора, в марте 2021 года, она работала в Grechka.Digital. Там регулярного ревью не было: задачи нужно было делать быстро, времени на проверку другим разработчиком не оставалось. Лиза связывала такой подход с коротким сроком жизни промосайтов, которыми часто занимаются студии.

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

Проверять код может тимлид, который хорошо знает устройство проекта и его задачи. Разработчики могут проверять друг друга. Существенно, чтобы человек понимал код, с которым работает, и мог объяснить свои замечания. Сама должность такого понимания не даёт.

Что можно поручить автоматике

Я спросил: если есть правила, почему бы разработчику не свериться с ними самому? Зачем тратить время другого человека на то, что уже описано?

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

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

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

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

Откуда берутся правила

Вопрос про субъективность ревью звучит примерно так: код проходит валидацию, но кому-то не нравится. Кто вообще решил, что нужно писать иначе?

Лиза предложила различать требования языка и правила конкретного проекта. У HTML, CSS и JavaScript есть спецификации. У команды есть договорённости, которые помогают работать вместе. Она может составить собственный код-гайд или взять за основу чужой, например руководство Airbnb для JavaScript. Но чужой документ всё равно приходится соотносить со своей работой.

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

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

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

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

Замечание, с которым можно работать

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

Лиза старается отмечать и удачные решения. Разработчику нужно понимать, что стоит сохранить, а не только что переделать. Правильно выбранные теги, продуманная сетка, последовательные отступы — всё это заслуживает конкретного комментария. Иначе после проверки остаётся впечатление, что в проекте вообще ничего не получилось.

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

Но одной пометки «нужно исправить» мало. Проверяющий видит проблему благодаря своему опыту, а автор этого опыта ещё не получил. Полезно объяснить, при каких условиях решение подведёт, и дать материал, который поможет разобраться. Лиза, например, собирает статьи о названиях цветовых переменных: несколько подходов позволяют увидеть причины выбора, а не заучить одно правило наставника.

Я считаю это важной частью работы ревьюера. Развёрнутое объяснение требует времени. Можно быстро пробежать по коду, оставить несколько указаний и пойти дальше. Но тогда человек узнает только, чего от него хотят сейчас. Если объяснить последствия решения, он сможет воспользоваться этим и в следующей задаче.

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

Не защищать каждую строку и не терпеть оскорбления

Лиза предложила начинать с предположения, что проверяющий заинтересован в хорошем результате. Я сразу оговорился: бывает и токсичное ревью. Если человек действительно пытается унизить автора, не нужно объяснять это заботой о качестве кода. Такое поведение стоит обсуждать прямо.

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

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

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

В Академии ученики проходят через похожий процесс: заранее знают критерии проекта, отправляют изменения наставнику и получают замечания в пулреквесте. О том, как мы организовали эту работу с задачами и проверкой изменений, я рассказывал в «Гитхабе в Академии». Письменные комментарии остаются рядом с кодом, к ним можно вернуться после разговора и проверить исправления.

Работа Ярослава: у каждого элемента есть назначение

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

В работе Ярослава она начала с шапки. Для неё использовался header, внутри были контейнер, навигация и ссылка с телефоном. Здесь nav обозначал навигацию, хотя для одной только раскладки можно было обойтись без него. Зато отдельный div вокруг ссылки выглядел лишним: самостоятельной задачи у него не было.

Похожий вопрос возник к display: grid у отдельных ссылок. Когда Лиза отключила это свойство, ничего не изменилось. Сетка у списка помогала расположить элементы, а у телефона и другой ссылки в данном случае не выполняла никакой работы. Замечание было не про запрет гридов для ссылок, а про конкретное свойство, которое оказалось ненужным.

В основной части Лиза отметила заголовки и структуру разделов. Автор использовал main, выделил секции, предусмотрел скрытый заголовок и следил за уровнями заголовков. В тексте были неразрывные пробелы — небольшая деталь, по которой видно внимание к типографике.

В блоке «О нас» picture был нужен для выбора формата изображения: помимо обычной картинки предусматривался WebP. Хорошо работала и договорённость об отступах. Они задавались последовательно снизу, поэтому при переходе от блока к блоку не приходилось разбираться, какая сторона отвечает за расстояние между ними.

Работа Яросвета: законченность и запас на будущее

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

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

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

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

В нумерованном списке автор использовал CSS-счётчик. Я предложил проверить его в действии: мы удалили второй пункт, и бывший третий получил номер два. Номера не пришлось исправлять вручную. Такая проверка заодно показывает, что решение рассчитано на изменение списка, а не только на то количество пунктов, которое было нарисовано.

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

Работа Марины: где обёртки не хватает

У Марины Гурьевой Лиза обратила внимание на структуру страницы: в ней не было main, который выделял бы основное содержимое. Затем перешла к отзывам. Их переключатели были картинками. На картинку можно повесить обработчик, но от этого она не получает всё поведение кнопки, в том числе работу с клавиатурой.

Здесь требовалось действие на месте — переключение слайда. Лиза предложила button. Для перехода на другую страницу или к разделу внутри текущей подошла бы ссылка. Такой выбор определяется действием, которое должен совершить человек, а не видом стрелки в макете.

После этого выяснилось, что у слайдера нет отдельной обёртки для одного отзыва. Я спросил, нельзя ли считать ею существующий контейнер reviews. Лиза объяснила: он включает и содержимое, и управляющие кнопки. Кнопки относятся ко всему слайдеру, а переключаться должна только часть с конкретным отзывом. Значит, у этих частей разные границы.

В начале разбора мы убирали лишние обёртки, а здесь понадобилась ещё одна. Универсальное правило «чем меньше div, тем лучше» не объяснило бы ни тот, ни другой случай. Нужно понять, что объединяет элементы и как эта группа будет работать.

В тексте отзыва абзацы разделялись переносами строк. Лиза предложила разметить их самостоятельными параграфами, а расстояния задавать стилями. Я рассказал, что в Академии мы как раз пересматривали критерии: вместо множества трудно трактуемых условий хотели явно запретить двойной br. Два переноса не должны заменять структуру текста и отступ между абзацами.

У фотографий не было alt, а сами они были сохранены в PNG. Лиза обратила внимание на вес изображений и предложила для этих фотографий JPEG. В работе Ярослава мы уже видели другой вариант доставки — через picture с WebP. Разговор шёл в 2021 году; тогда я отмечал, что внедрение формата зависит не только от браузеров, но и от того, насколько удобно получать нужные файлы из графических редакторов.

Форма и подвал

В форме поля располагались с помощью br, хотя для самого form раскладка не была задана. Лиза предложила расположить содержимое формы колонкой средствами CSS. Отдельная внешняя обёртка тоже выглядела необязательной.

В атрибуте type у полей стояли значения form_name и form_phone. Я предположил, что Марина перепутала его с name: для имён полей эти значения имеют смысл. Тип же описывает поле ввода — например, text или tel. Имя позволяет связать переданное значение с нужным полем при обработке формы.

Лиза также обратила внимание на тип кнопок. У отправки формы должно быть явное назначение submit, у переключателей отзывов — button. Это помогает не смешивать разные действия, даже если внешне элементы похожи.

В подвале была ещё одна обёртка, повторяющая сам footer, а телефон оставался текстом. Его можно было сделать ссылкой с tel:, чтобы пользоваться им с устройства, которое поддерживает звонки.

Расстояние между частями подвала задавалось большим фиксированным отступом. Я попросил Лизу объяснить, почему она предлагает автоматический. Сейчас рядом помещались телефон и адрес. Но у компании мог появиться второй адрес, реквизиты или ещё какой-то текст. Фиксированный промежуток продолжил бы занимать столько же места, а содержимому пришлось бы тесниться. Автоматическое распределение свободного места лучше учитывало это изменение.

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

Ещё одна работа: проверить заменой содержимого

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

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

Зато ниже, в блоке с формой, проблема проявилась. Картинка тоже стояла абсолютно, а ширина текста рядом с ней ограничивалась отдельно. У заголовка такого ограничения не было. Когда его удлинили, он ушёл под изображение. Лиза предложила собрать текстовую часть в контейнер и расположить её рядом с картинкой через общую сетку.

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

Для этой плиточной композиции Лиза предложила Grid: задать колонки и распределить карточки между линиями сетки. Так можно было описать их отношения, не фиксируя каждую ширину отдельно.

Слайдер в этой работе уже был разделён на самостоятельные слайды. Отзыв размечался как цитата с автором, у картинок были альтернативные тексты, а у переключателей — тип button. В форме типы полей тоже были выбраны, но отсутствовали имена. Ещё Лиза заметила, что подсказки внутри полей почти сливались с фоном. Правильная структура не избавляет от проверки того, видит ли человек нужный ему текст.

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

Перенос из макета не всегда нужен в тексте

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

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

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

Лиза объяснила, что делает скидку на начинающего автора, который стремится попасть в макет. Я с этим не спорил: ошибка в учебной работе понятна. Мне было важно отделить её от решения, которое можно рекомендовать на будущее. Мы только что обсуждали изменяемость слайдера и подвала; к тексту относится то же требование.

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

Внешний взгляд нужен и тому, кто проверяет

В конце один из участников спросил о работе самой Лизы: почему в её варианте макета нельзя выделить текст на уровне стрелок слайдера? Она предположила, что управляющие элементы перекрывают текст, и допустила, что ошиблась с наложением слоёв. Точную причину в разговоре не установили.

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