Инспектирование кода: лучшая практика

Рассказывает Кевин Лондон, автор блога kevinlondon.com

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

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

На что смотреть во время инспекции

Архитектура/Дизайн

• Принцип «одной ответственности». Идея в том, что у каждого класса должно быть только одно назначение. На самом деле реализовать это труднее, чем кажется. Я обычно применяю это правило и к методам. Если возникает нужда в союзе «и» при описании того, что делает метод, то это знак, что стоит разделить его на несколько более простых.
• Принцип «Открыт/Закрыт». Если язык объектно-ориентированный, то открыты ли ваши объекты для расширения, но закрыты для модификации? Что произойдет, если нам нужно будет добавить еще один экземпляр класса x?
• Дупликация кода. Я придерживаюсь правила «трех раз». Если код повторяется один раз, то я закрою на это глаза, хоть это и выглядит некрасиво. Но если один и тот же кусок используется три раза или больше, то необходимо вынести его в отдельный метод.
• Слепой тест. Если расфокусировать зрение, то выглядит ли форма кода, на который вы смотрите, идентичной другим кускам кода? Если нет, то это может быть сигналом к тому, что код нуждается в рефакторинге.
• При изменении кода всегда пытайтесь его улучшить. Обычно, если я исправляю какую-то часть кода, которая работает неправильно или выглядит некрасиво, у меня всегда возникает искушение просто исправить несколько строчек и на этом закончить. Я рекомендую не останавливаться на этом и сделать код лучше, чем он был.
• Потенциальные баги. Просматривайте код на наличие ошибок-на-единицу, нарушений условий циклов и т.д.
• Обработка ошибок. Достаточно ли хороша обработка ошибок? Введены ли кастомные исключения? Если да, то полезны ли они?
• Эффективность. Если используется какой-либо алгоритм, эффективна ли его реализация? Например, пробег по всем ключам словаря — не лучший способ найти нужное значение.

Стиль

• Имена методов. Давать имена различным вещам — одна из самых сложных задач в программировании. Если метод называется get_message_queue_name(), но делает что-то кроме этого, например, убирает HTML из входных данных, тогда это имя не подходит ему.
• Имена переменных. foo и bar — не самые лучшие имена для структур данных. e также не настолько красноречиво, как exception. Будьте как можно лаконичнее. Говорящие имена переменных облегчат чтение кода в будущем.
• Длина функций. Я придерживаюсь правила, говорящего, что функция не должна быть длиннее 20 строк. Если я вижу метод больше 50 строк, я пытаюсь разбить его на два.
• Длина классов. Классы должны быть меньше 300 строк, а в идеале — меньше 100. Скорее всего, если в вашем коде есть длинные классы, то их можно разбить на несколько, что облегчит понимание их предназначения.
• Длина файла. Для Python 1000 строк в одном файле — предел. Если их становится больше, то, возможно, стоит разбить файл на несколько, с более специфичным предназначением. Чем больше файл, тем меньше читабельность кода в нем.
• Документация. Сложные методы лучше задокументировать так, чтобы было понятно, за что отвечает каждый аргумент. Разве это не очевидно?
• Закомментированный код. Стоит удалить закомментированные строки кода, чтобы не было ничего лишнего.
• Количество аргументов в методе. Посмотрите, сколько аргументов передается в ваш метод. Больше трех? Это знак того, что они могут быть сгруппированы по-другому.
• Читабельность. Легко ли разобраться в вашем коде? Часто ли вы делаете паузы во время чтения, чтобы разобраться в нем?

Тестирование

• Полнота тестов. Мне нравится, когда нововведения тестируются. Но насколько эти тесты продуманы? Могут ли они заставить ваш код упасть? Легко ли они читаются? Насколько они хрупки? Насколько они большие? Медленные ли они?
• Тестирование на правильном уровне. Когда я рассматриваю тесты, я должен убедиться в том, что они проводят тестирование на правильном уровне. Иными словами, тестируется ли тот уровень приложения, который нужно тестировать для проверки функциональности? Гарри Бернардт рекомендует такое соотношение — 95% юнит-тестов и 5% интеграционных тестов. Особенно это относится к проектам, использующим Django.
• Количество объектов-имитаций. Имитационные объекты хороши, но не стоит пихать их везде. Если в тесте их более трех штук, нужно его переписать. Либо тест, либо сама функция, для которой он предназначен, охватывают слишком большую часть кода. В обоих случаях это стоит обсудить.
• Соответствование требованиям. Обычно в конце инспектирования я смотрю на задачу или баг, для которого был предназначен тест. Если он не соответствует каким-то критериям, то лучше провести тестирование заново.

Сначала проинспектируйте свой код сами

Перед совершением коммита просмотрите свой код на следующие вещи:

• Оставили ли вы какие-нибудь комментарии или TODO?
• Говорят ли сами за себя имена переменных?
• …что угодно из того, что я перечислил выше?

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

Как проводить инспектирование кода

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

• Задавайте вопросы. Задавайте вопросы, которые подтолкнут разработчиков к обсуждению. Например, как работает этот метод? Если изменится какое-то требование, то что нужно будет поменять в коде? Как сделать код более поддерживаемым?
• Делайте комплименты и поощрения. Одна из самых важных вещей в тестировании – награждение разработчиков за рост и приложение усилий. Немногое может сравниться с похвалой от тимлида. Я стараюсь сделать настолько много положительных комментариев, насколько это вообще возможно.
• Обсуждайте детали наедине. Большие архитектурные изменения лучше обсуждать всей командой, в то время как про мелкие детали лучше говорить наедине с разработчиком, который ответственен за них, дабы не вовлекать лишних людей.
• Объясняйте причины. Всегда лучше рассказать или спросить, почему предложенные изменения необходимы. Порой может возникнуть чувство, что они несущественны, до тех пор, пока вы не объясните повод.
• Дело в коде. Обсуждайте сам код, а не разработчиков, которые его писали. Это создаcт непринужденную атмосферу, тем более, программисты ни при чем — инспектирование призвано улучшить качество кода.
• Указывайте на важность изменений. Обычно я предлагаю множество изменений, не все из которых обязательны. Если вы считаете, что ваше предложение важно, то стоит сказать об этом, тогда на него обратят внимание и быстрее начнут двигаться в нужном направлении, что создаст видимость результата.

Не забывайте о своих обязанностях

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

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

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

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

Дополнительно

Существует несколько книг о написании хорошего кода, которые я советую вам прочитать. Вот они:

• «Чистый код»
• «Рефакторинг»

Также рекомендую несколько видео, идеи из которых я держал в уме, когда писал эту статью:

• «Мелочи»: отлично рассказывает о теме, частично с перспективы написания чистого, пригодного к использованию кода;
• «Как спроектировать хорошее API и почему это важно»: API, в частности и то, как его спроектировать и взаимодействовать с ним. Обсуждаются многие вещи, о которых говорилось в этой статье.

Перевод статьи «Code Review: Best Practices»