Ситуация глазами разработчика
Ты написал код, отправил его на ревью. Во время ревью тебе указали на ошибку в коде и вернули задачу на доработку.
Здесь мы не рассматриваем ошибки в бизнес-логике написанного кода — только ошибки на уровне интерпретатора/компилятора, из-за которых приложение «падает».
Чего НЕ нужно делать
Не нужно послушно исправлять указанную ошибку и отправлять задачу на повторное ревью. После чего ждать дальнейшей участи и надеяться на то, что ревьюер ничего больше не найдет и пропустит код дальше.
А еще не нужно оправдываться. Оправдание есть у каждого (открою секрет: ревьюеру не интересно, почему ты допустил ошибку. В первую очередь это должно быть интересно тебе лично — сделай правильные выводы, чтобы такая ошибка не повторялась).
Что нужно сделать
Не нужно исправлять ошибку сразу. Проверь работоспособность всего кода. Повторю — ПРОВЕРЬ РАБОТОСПОСОБНОСТЬ. И исправь ВСЕ выловленные ошибки.
Пройдись по коду всеми возможными линтерами и анализаторами (линтер - это как автоматическая проверка орфографии в MS Word, только для кода).
Запусти код еще раз у себя (локально, на тестовом стенде, еще как-нибудь).
«Погоняй» код на различных данных. Пообщайся с постановщиком или тестировщиком, чтобы получить приближенные к реальности входные данные для тестирования.
Поправь найденные ошибки.
Повтори все вышенаписанное еще раз.
Почему?
Наличие ошибки — это повод задуматься о том, что код плохо протестирован самим разработчиком. А ведь прямая обязанность разработчика — это в первую очередь предоставление РАБОТОСПОСОБНОГО кода.
Тестировщики проверяют корректность бизнес-логики, граничные и нестандартные значения. Но только при условии, что код работает и способен запуститься на тестировочном стенде.
Повторяю — нужно исправлять не ошибку, а проверить работоспособность своего кода. Каждой его строчки. И убедиться, что твой код не повлиял на другой функционал.
Запомни: твой коллега-ревьюер — не линтер. В его обязанности не входит построчная проверка кода на синтаксические ошибки.
И тестировщик не линтер.
Цель ревью — взглянуть на твое решение «свежим» взглядом или с позиции опыта и указать на моменты реализации, которые ты не учел. Но никак не поиск ошибок на уровне кода.
А задача тестировщика — проверить код на соответствие бизнес-требованиям, а также убедиться, что новый функционал не повредил стабильные узлы.
Конечно, есть компании, где ревьюеры пробегутся по всему коду и подробно укажут на все найденные ими ошибки. Но такое бывает редко. И такие ревьюеры — тема для отдельного поста.
Вывод
Ситуации, когда от разработчика выходит неработоспособный код, конечно же бывают. Никто не отменял запары, форс-мажоры, усталость и невнимательность. Но если такие ситуации повторяются регулярно, то это существенный удар по твоей репутации как разработчика.
Ревьюерам не хочется постоянно быть нянькой — им хочется выполнять СВОИ задачи, а вместо этого они отвлекаются на поиск ошибок в ТВОЕМ коде.
И это повод:
Обновить свой инструментарий: обвесить свою IDE (или редактор кода) линтерами и анализаторами или настроить скрипт, который при изменениях прогонял бы код проекта через них.
Подумать над средой для тестирования задач. Чтобы можно было собрать проект и «погонять» его при разных условиях. Не так глубоко, как это делают тестировщики. Но основные кейсы нужно обязательно протестировать самостоятельно. Лайфхак: спроси у коллег, как они тестируют свой код перед отправкой в ревью.
При постановке задачи сформировать (с постановщиком, тестировщиком или самостоятельно) тест-кейсы.
Ревьюер скажет тебе спасибо, если ты коротко опишешь список изменений, которые ты внес в код. А также укажешь, как именно тестировал и проверял свой код.
Подумать о внедрении подходов и инструментов тестирования (Unit-тесты, TDD, например, если их нет) лично для себя или на уровень всего отдела.
Ситуация глазами редактора
Ты написал текст, отправил на проверку и получил фидбэк о пропущенной запятой или орфографической ошибке.
Нет.
Исправить ошибку и снова отправить на проверку.
Да.
Внимательно — весьма внимательно — пройтись по всему тексту и перепроверить все запятые и буквы.
Почему?
1
Время принимающего текст (например, старшего редактора) для бизнеса стоит дороже. Он может либо править твои ошибки, либо делать более сложную задачу.
2
Страдает твоя личная репутация. Неимоверно бесит два или три раза подряд отправлять текст на правку элементарных вещей.
3
Возможно, в тексте есть и другие — более серьезные ошибки. Если ты дописываешь текст в сильной запаре и с вытекающими под вечер глазами, остановись. Проветрись и проверь еще раз. Может, где-то дублирование смыслов. Может, неверная ссылка. В общем, проверь.
Как быть?
Не страшно пропустить запятую. Важно отношение. Не надо писать, что было мало времени на вычитку. Или что голова распухла от задач. Надо принять замечание и все внимательно проверить.
Хитрость
Если ситуация такова, что надо срочно отправить текст, который дописываешь с гудящим от напряжения мозгом, пиши коммент: «Отправляю текст. В нем могут быть опечатки, потому что делали срочно. В течение часа я внимательно проверю еще раз».
Вывод
Пропущенная запятая — не ошибка, а сигнал о повторной тщательной проверке.
P.S.
То же относится, когда на ошибку указали в комментариях в соцсетях. Поблагодарить за внимательность, исправить ошибку и пойти внимательно читать весь текст. Весь. Текст. Целиком.
Комментарии (23)
mixtyraa
26.02.2022 18:53+5Вроде простая тема, банальная даже, но описанная проблема действительно существенная среди junior разработчиков. Спасибо за попытку привлечь к этому внимание.
К данной теме я могу ещё отнести кейс, когда спрашиваешь разработчика - почему он пошёл таким путем, а не вот таким (либо использовал функцию А , вместо функции Б), и вместо ответа получаешь коммит с исправлениями.
Такое поведение чаще всего встречаю у джунов, и здесь проблема в основном в том, что они не уверены в своих силах, своей компетенции, к любым вопросам по коду относятся как к претензии , замечанию , которое необходимо исправить. Тоже самое и с комментариями об ошибках. Такое поведение встречается и среди мидлов.
Положение не исправить настроенными линтерами, unit тестами, tdd. Линтеры хорошо, помогают находить ошибки, но нужно тоже понимать, что они советуют, и действительно ли это верный совет. Если юнит тестов нет, то джуниор/мидл , никак их не внедрит в проект. А тимлид или менеджер будет слышать только необдуманные идеи со стороны разработчика. Дело не в том, что разработчик не разберётся в этой компетенции (юнит тестах, tdd), просто он не сможет защитить свою идею, раз мы уже имеем проблему, что разработчик правит ошибки не думаю о том, а действительно ли это ошибка.
Развитие сотрудника - это не только развитие в нем технических компетенций. Нужно учить и работе над ошибками, что вы и старались заложить в основу статьи, да, не нужно необдуманно править все подряд, нужно уметь сделать какие-то выводы, проработать проблемы, если такие есть, уметь защищать своё решение, но входя в открытую дискуссию, понимать, что цель не выиграть спор, а найти истину. Научить человека воспринимать комментарии на ревью , как призыв к общению, обсуждению, а не как проблему, которую необходимо исправить прямо сейчас, учить разработчика вступать в дискуссии, развивать в нем софтскиллы.
andrey_and_pavel Автор
26.02.2022 19:21+2К данной теме я могу ещё отнести кейс, когда спрашиваешь разработчика - почему он пошёл таким путем, а не вот таким (либо использовал функцию А , вместо функции Б), и вместо ответа получаешь коммит с исправлениями.
Вот да. То же и к тексту относится. Бывает, оставишь в гугл-доке коммент с вопросом, а потом видишь, что твое замечание снято и текст переписан. Но вопрос-то остается: "Почему такое решение?" Думаю, важно не бояться диалога. И не бояться говорить о своей позиции в выборе решения. Позиция, даже если она иногда неверная, лучше, чем ее отсутствие.
SergeyMax
26.02.2022 20:15Странный вопрос "почему такое решение". Потому что так придумал, и так сделал, вот почему.
andrey_and_pavel Автор
26.02.2022 20:32+3Вполне нормальная позиция.
CrocodileRed
27.02.2022 11:35+1Для овоща на грядке :-)
Задавать вопросы почему и зачем необходимо. Без этого "я так придумал" будет основным аргументом, а он плохо работает при общении с коллегами.
andrey_and_pavel Автор
27.02.2022 12:55Так в ответ на "Я так придумал" последует другой вопрос: "Почему так придумал?" Нормальный конструктивный диалог.
CrocodileRed
27.02.2022 12:58+1Если сценарий заранее известен, тем более модно опустить его и перейти сразу к сути :-)
andrey_and_pavel Автор
27.02.2022 13:07Начинающий специалист может не знать о том, что в команде принят такой сценарий. И при следующем ревью сразу перейдет к сути)
panteleymonov
27.02.2022 10:43+1На самом деле очень часто приходиться костылить на скорую руку, и после того как твой костыль нашли, уже родился боле менее вразумительный результат или свежий взгляд на проблему привел к решению. Поэтому объяснять каждый раз, что это затычка, которую собирался доработать, вроде и надоело и стыдно и должно быть понятно. Тут можно развести отдельный холивар, что «надо всегда писать чистый код», но даже у сеньоров в реалиях разработки это не всегда получается, а такие заявления чаще слышно от не практикующих или пишущих одно и тоже. И стоит таким правильным товарищам выйти из своей зоны комфорта как тут же становятся и мидлами и джунами. А вместо уверенности выпирает «требую уважения» и прочая токсичность. Но если вопрос все еще остается, то возможно и так, что вы чего-то не знаете. Ведь знать абсолютно все в принципе невозможно. Хотя оставлять вопросы без ответа плохая практика.
И еще я очень часто сталкивался с таким моментом, что ответ может повлечь за собой только еще больше вопросов, особенно когда этот товарищ не из твоей команды. Это приводит к разведению холиваров и отвлечению от работы, поэтому иногда лучше все-таки промолчать.
andrey_and_pavel Автор
27.02.2022 13:06И стоит таким правильным товарищам выйти из своей зоны комфорта как тут же становятся и мидлами и джунами.
Ну да. Любой может на время стать «джуном» в новом проекте. Или с новым продуктом.
И еще я очень часто сталкивался с таким моментом, что ответ может повлечь за собой только еще больше вопросов, особенно когда этот товарищ не из твоей команды. Это приводит к разведению холиваров и отвлечению от работы, поэтому иногда лучше все-таки промолчать.
Так наша основная мысль в том, что указание на ошибку или вопрос о решении — это сигнал. Сигнал, что нужно еще раз внимательно все перечитать и проверить (если мы про текст). Указание на ошибку или вопрос о решении — это не оскорбление. А просто знак, говорящий о том, что надо обратить внимание на отдельный кусок текста или на весь текст целиком (или код). Либо понимать, что этот кусок - костыль, и в будущем он может аукнуться.
aamonster
27.02.2022 13:38К костылю лучше б сразу в комментах пояснение оставлять (в т.ч. об условиях работы данного костыля).
А то когда в нём баг – это полбеды, беда, когда через несколько лет смотришь на этот код и не можешь понять, зачем он нужен, надо его править под новые условия или можно просто стереть (даже не переписать заново: это мог быть, к примеру, обход для давно не поддерживаемой версии ОС).
CrocodileRed
27.02.2022 11:29Я бы заметил, что цель дискуссии не поиск истины (для меня, по крайней мере), а способ выяснить и обсудить альтернативные точки зрения. А уж "истину" после этого сам каждый сам для себя определяет.
CrocodileRed
27.02.2022 11:18Золотые слова. Распечатать и повесить всем перед лицом на рабочие места :-)
SamMetalWorker
27.02.2022 17:13твой коллега-ревьюер — не линтер ... И тестировщик не линтер.
Да и разработчик так-то не линтер, если на проекте возникают такие проблемы, почему бы линтер не сделать частью CI, чтобы сразу отпинывать плохой код и не тратить время?
andrey_and_pavel Автор
28.02.2022 11:16Все верно, согласен. Автоматизация проверок - это очень хорошая и нужная практика. Если она внедрена в компании, то становится гораздо проще и легче.
А если такой практики нет, то нужно поднимать вопрос о ее внедрении. А по моему (субъективному) опыту в некоторых компаниях с ходу это сделать довольно проблематично. Поэтому новому сотруднику лучше сперва начать с себя, а затем перенести эту практику на уровень отдела/компании.
Anshi85
28.02.2022 05:32+1Со временем сложились правила:
Проверерять код на работоспособность вручную.
Проверять работоспособность кода запуском тестов
Прогонять через линтер написанный код.
Без этих шагов отправлять на "код ревью" нельзя, ибо это неуважение к проверяющему отдавать на проверку нерабочий код.
panzerfaust
Лайфак года просто. На выходе будет ситуация, где у трех разрабов четыре мнения о тестировании. Такие вещи должны быть описаны в конфе на старте проекта. Ну или не на старте. Но новичок точно не должен бегать в мыле и выяснять что к чему. Там же должно быть все о линтерах, форматировании, нейминге, паттернах и т.д.
amarao
Во всех используемых языках? Даже внутри регэкспа внутри lua-скрипта внутри конфига веб-сервера внутри jinja-шаблона внутри роли ансибла?
А вдруг, коллега знает, как такое проверять?
CrocodileRed
А инструкция по взаимодействию с туалетной бумагой вам тоже нужна?
andrey_and_pavel Автор
Если все описано в Конфле, то у трех разработчиков уж должно быть единое мнение)
andrey_and_pavel Автор
Год только начался, поэтому таких лайфхаков будет еще немало.
Такие вещи должны быть описаны в конфе, согласен. Но если бы все было так просто, то тогда наша статья состояла из одного предложения: "Если в вашем коде нашли ошибку, то внимательно почитайте конфлюенс и исправьте ее".
Если человек пришел в компанию, где есть хорошая база знаний и где хорошо работает система погружения, то вопросов нет.
Но на деле (на основе субъективного опыта авторов статьи) бывают компании, у которых либо нет базы знаний, либо она устарела. И получаются ситуации, когда код написан, передан в ревью, но не протестирован.
И в этом случае, пусть лучше у новичка будет 4 мнения от 3 разработчиков, чем непротестированный код в ревью.