С утра вы приходите на работу, выпиваете чашечку кофе, закусывая печенькой, и в полной боевой готовности идете на свое рабочее место. Заходите в Jira (любой другой трекер), выбираете наиболее приоритетную задачу из текущего спринта и переводите ее в статус “In Progress”. Спустя некоторое время увлеченной работы задача отправляется на код-ревью, а вы позволяете себе отлучиться еще за одной чашкой кофе.
Вы довольны собой – к решению задачи вы подошли ответственно и копнули вглубь, проработав все возможные сценарии развития событий и написав красивый и лаконичный код. Вы возвращаетесь с кухни спустя несколько (десятков) минут, отвечаете на пару сообщений в одном из чатов и замечаете, что ваш почтовый ящик не пуст. Это пришла нотификация с гитхаба – ваш ревьюер оставил около 10 комментариев и сделал change request.
Знакомая ситуация?
Вы открываете свой Pull Request и видите все эти комментарии. Вы начинаете разбираться – первые 3-4 комментария требуют тривиальных правок, и вы начинаете думать, что на самом деле особых проблем в вашем коде нет. Пока не пролистываете change request до конца.
Ревьюер требует конкретного изменения логики в том месте, которым вы больше всего гордились. Вы испытываете смешанные эмоции – сначала “что это такое?”, затем “ну хорошо, а чем мое решение хуже?”, а затем и вовсе не согласны с ревьюером. Вы начинаете его ненавидеть за его занудство и дотошность. В конце концов, вы же самостоятельный программист, каким подобает предлагать и воплощать свои собственные решения. Даже простейшую задачу можно решить множеством способов – почему же ревьюер требует, чтобы вы реализовали именно его подход, хотя ваш ничем не хуже? Возможно, вы даже начинаете думать, что ваш ревьюер уровнем-то пониже вас будет.
Однако, вы честно пытаетесь понять, почему он оставил такие комментарии, и копаете глубже. Само собой, у вас не должно быть никаких оснований для недовольства, если вы написали что-то вроде этого:
switch (object.getField()) {
case "value_1":
// Ага, мы сюда попали по value_1, значит это такой-то кейс
if (condition_1 ||
condition_2 ||
condition_3 ||
condition_4) {
// some actions
} else if (condition_5) {
// another actions
} else if (condition_6) {
// another another actions
} else if (another_fucking_condition) {
// another fucking actions
} else {
log.warn("Let's just write this warning: {}", "some_explanation");
}
break;
case "value_2":
// Ну, value_2 говорит совсем о другом кейсе
// some actions
break;
case "value_3":
case "value_4":
case "value_5":
case "value_6":
// do some common staff
break;
}
Если вы пишете такое, то это говорит либо о том, что вы хреновый программист, либо о том, что вы просто не паритесь и не вкладываетесь в проект. Заработало – здорово. Тут у ревьюера есть все основания ткнуть вас носом в ваш же говнокод. Начав с совершенно идиотских кондиций вроде (condition_1 || … || condition_n), которые можно либо упростить, либо вынести в отдельный метод conditionsSatisfied() (нужно обдуманное название), и заканчивая тем, что для таких случаев здоровые люди используют фабрики. В итоге весь код сводится к следущему:
SomeFactory.get(object.getField()).execute(...);Само собой, в данном случае создаются три конкретных реализации, для каждой из возможных веток. Не буду углубляться в этот пример далее, потому что я хотел сказать чуть о другом. Если вы пишете такое – то претензии должны быть только к себе.
Допустим, у вас есть два модуля – в одном вы ориентируетесь как рыба в воде, а в другом – как в лесу. Однако, у вас есть доступ к коду второго, и вы можете его править. Рассмотрим исходные условия этого примера:
Дано:
- 2 модуля, один из которых шлет сообщение другому. Допустим, через REST запрос, хотя это не важно.
- Есть сущность в базе данных, которая отслеживается в обоих модулях.
- Модуль первый (ваш) в какой-то момент понимает, что объект больше не нужно отслеживать. Он шлет запрос во второй модуль и просит его сделать соответствующие изменения в базе.
Первые 2 пункта – чисто вводная, вся соль в третьем.
Вы пишете элегантный и красивый код. Вы не изобретаете велосипедов и используете именно ту http-библиотеку, которую принято использовать на вашем проекте (возможно, это даже не 3rd-party, а еще один внутренний компонент). Во втором модуле вы создаете отдельный package, который ответственен за общение именно с вашим модулем, а также создали роутинг. Покопавшись в коде модуля, вы с радостью находите метод, который называется весьма обнадеживающе – untrack_object(...). Вы смотрите тело метода и обнаруживаете, что это именно то, что вам нужно. В конце концов, все чертовски логично – если объект отслеживается, а следовательно, есть метод start_tracking(...), то должен быть и обратный сценарий. В итоге у вас получается что-то вроде этого (псевдокод):
@route('/resource/untrack/<id>', methods=[‘POST’])
@require_login
@another_annotation
def untrack_object(user, id):
o = get_entity_by_id(id)
Tracker.untrack_object(o)
Entity.delete(o)
notify_users_about_deletion(user, o)
return jsonify(ok=True)Вы пишете юнит-тест, тестируете руками – все работает. Объект больше не отслеживается и удаляется из базы, но ревьюер все равно с чем-то не согласен. А коммент его следующий: «Нельзя просто так взять и удалить объект. Необходимо пометить его как архивный, а не удалять из базы – он может еще пригодиться в будущем. А также необходимо произвести еще некоторые подготовительные действия». В комментарии он пишет вам опять-таки псевдокод:
def untrack_object(user, id):
o = get_entity_by_id(id)
Tracker.archive(o)
o.archive()
some_other_actions_with_o
and_more
notify_users_about_deletion(user, o)
return jsonify(ok=True)Вы понимаете, что он прав, но все равно негодуете: почему он вам пишет какой-то псевдокод, хотя самому ему написать рабочий код совершенно не составит труда? Он напишет это за пять минут, а у вас может уйти полчаса на то, чтобы разобраться и переделать.
Но делать нечего, и вы начинаете разбираться. Спустя полчаса работы вы:
- Понимаете, что ваше изначальное решение действительно рушило логику приложения.
- Изучили еще одну крупинку второго модуля и стали в нем чуть лучше ориентироваться.
- Вероятно, вам пришлось полазить в документации слабо знакомого вам языка, что опять-таки на крупинку улучшило ваше им владение.
- Все еще ворчите на ревьюера, но глас рассудка подсказывает вам, что причин для этого нет.
Само собой, “ненависть” – лишь образное выражение, вы не должны и не будете ненавидеть его в прямом смысле. Однако, если время от времени у вас появляются негодования, когда вы видите очередной change-request с комментариями, вы должны понимать, что это только улучшит ваше знание системы и сделает вас более ценным разработчиком. Так что злитесь, “ненавидьте”, сходите в спортивную комнату и отлупите грушу, но затем успокойтесь и сделайте все как надо :)
Автор: Артём
