Писать код сложно. Проверять его тоже сложно. В моей команде каждый тикет пристально рассматривается второй парой глаз.
Почему? Работа, выполнененная через жопу, может заставить компанию выглядеть плохо, при этом терять деньги, терять клиентов, генерировать стресс и дополнительную работу, поэтому обязательно делать хороший код-ревью, прежде чем запускать код в жизнь.
В течение последней пары месяцев я составляла свой собственный чеклист проверки кода. Я использую его как для проверки своего собственного кода, так и для тикетов, закрытых моей командой. Весь список делится на 3 раздела: написание кода, автоматическое тестирование и ручное тестирование.
Код
- Он компилируется? Ищите очевидные ошибки, большинство IDE помогут вам в этом. Непонятно, как такого рода проблемы проскальзывают в завершенный код, но все мы люди, и ошибки случаются. Самые распространенные причины: в последнюю минуту добавляется без тестирования код, который «конечно же ничего не сломает» или разработчик забывает загрузить исправление.
- Можете ли вы предвидеть какую-нибудь рантайм-ошибку просто посмотрев на код? Попробуйте воспроизвести ошибку.
- Завершена ли фича или исправление бага? Выполнены ли все требования? Есть ли что-то отсутствующее или работающее неправильно?
- Это хорошее решение? Это лучшее решение из всех, что были обдуманы? Это больше относится к архитектуре системы и общему подходу решения проблем. Определение «хорошего» и «лучшего» зависит от контекста.
- Открывает ли это какие-нибудь дыры в безопасности? Безусловно, нельзя ничего игнорировать.
- Эффективен ли код в любых показателях, которые имеют для вас значение.
- Корректен ли он логически? Он решает проблему? Это обычно самое важное в код-ревью, также очевидное, но я не хочу пропускать очевидные вещи.
- Как он влияет на остальную часть системы? Мы просматриваем разницу кода при помощи diff в минимальном контексте (или вообще без него) влияния на остальной код. Используя diff вы можете увидеть только то, что изменили, поэтому как насчет того, чтобы начать смотреть в другие места? Думайте о каждом компоненте, который может зависеть от этого изменения. Где-то нарушается работа какого-нибудь API?
- Выходит ли он за рамки своих задач? Затрагивает ли тикет какую-нибудь несвязанную часть кода? Делает ли он сторонние исправления? Чем больше он засоряется, тем сложнее становится его проверять, тестировать и релизить. Поэтому, не отходите от сути.
- Есть ли какие-нибудь ошибки в оформлении кода? Соглашения важны для сохранения разумности кода. Я знаю, мы все умные люди и сами все знаем, но ради вашей команды и благополучия кода, следуйте правилам оформления кода.
- Он читабелен? Может ли кто-нибудь другой понять его? Сколько времени вам понадобилось, чтобы понять это? Все ли именования имеют смысл?
- Имеется ли у него хорошая и достаточная документация? Нельзя это игнорировать или считать неважным.
Автоматическое тестирование
- Нуждается ли он в тестах? Иногда можно легко справиться с заменой favicon без написания тестов.
- Есть ли у него тесты? Без шуток, пишите тесты.
- Правильны ли логически тесты? В тестах тоже могут быть баги. Проверьте их тщательно.
- Насколько просты, читабельны и отлаживаемы тесты? Забудьте про DRY, не разрабатывайте целую инфраструктуру, чтобы протестировать свой код (если только вы не пишите изолированный фреймворк для тестирования с чистым API, с которым все другие члены вашей команды более-менее знакомы), KISS (keep it simple, stupid — не усложняй это, болван). Если что-то ломается, должно быть понятно, что именно.
- Правильно ли тестировать ради тестов? Тесты — это не галочки, которые надо отмечать, они являются основой вашей безопасности. Простое существование тестов или высокого покрытия тестами не гарантируют жизнь, свободную от багов. Убедитесь, что вы получаете хорошую подстраховку на твердой основе тестов.
- Тестируются ли редкие случаи? Тестирование лучших сценариев поведения — задача для сосунков!
Ручное тестирование
- Вы можете запустить код? Не пропускайте этот шаг. Автоматические тесты выглядет хорошо на бумаге (ммм, или мониторе компьютера?), но запустить свое приложение — все равно неплохая идея. Автоматические тесты сводят к минимуму ручные тесты, но не надо их исключать полностью.
- Можете ли вы пройти несколько тестовых сценариев? Выберите очевидные и неочевидные: достигают ли они успеха?
- Выглядит/работает ли остальная система хорошо? Этот пункт похож на «Как он влияет на остальную часть системы?» из раздела Код. Но этот более прямолинейный. Покликайте вокруг и убедитесь, что все в порядке.
- Вы можете сломать его? Подумайте о причинах, по которым он может сломаться: накормите его большим набором данных, учтите понижение производительности из-за сторонних систем и как это может повлиять на вас, используйте необычные email. Не беспокойтесь, ваши пользователи будут любезно продолжать находить способы сломать его ради вас.
На этом все. Не стесняйтесь предлагать свои пункты 🙂
Статью перевел aziev. Оригинал на Ana-balica.github.io доступен по ссылке.