05/06/2017 · Разработка

Почему ненавидеть код-ревьюера – хорошо

С утра вы приходите на работу, выпиваете чашечку кофе, закусывая печенькой, и в полной боевой готовности идете на свое рабочее место. Заходите в 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 пункта – чисто вводная, вся соль в третьем.

Вы пишете элегантный и красивый код. Вы не изобретаете велосипедов и используете именно ту 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 с комментариями, вы должны понимать, что это только улучшит ваше знание системы и сделает вас более ценным разработчиком. Так что злитесь, “ненавидьте”, сходите в спортивную комнату и отлупите грушу, но затем успокойтесь и сделайте все как надо :)

comments powered by Disqus