Code review: вредные советы для контрибьютера и ревьювера
Привет! Меня зовут Николай Ижиков. В этом посте я хочу рассказать об одном важном элементе взаимодействия, с которым мы сталкиваемся в процессе разработки ПО, особенно в open source. Это прохождение и проведение code review.
Я дам вредные советы, как сделать свою фичу и довести ее до merge в мастере opensource-проекта в условиях культурных, временных, понятийных и прочих различий между участниками сообщества. Обычно это тонкий вопрос, особенно для тех, кто только начинает работу в open source.
Начнем с небольшой вводной. Общаясь с людьми, особенно в чате или по почте, тем более на английском языке, всегда учитывайте, что ваше сообщение может быть воспринято совершенно не так, как вы предполагали. Кто прочтет письмо — неизвестно. Он может быть индусом, англичанином или просто невыспавшимся человеком. В понимании конкретных слов всегда будут различия, и я, не вдаваясь в технические подробности, расскажу, как свести их к минимуму.
Предупреждение: рассказ содержит сарказм.
Вредные советы для контрибьютера
Ни с кем не обсуждайте новую фичу
Если вы захотели реализовать новую фичу, ни в коем случае ни с кем не обсуждайте свою доработку. Кто-то может подслушать и сделать это раньше вас! Или, услышав подробности, вам могут сказать, что вы чего-то не поняли — покритиковать вашу идею. Критика сильно ранит. Возможно, такую фичу как ваша уже обсуждали, и нужна другая… В общем, ни в коем случае никому не рассказывайте, что вы хотите сделать. Никогда.
Никогда не делайте техническую документацию
Коммиттеры и опытные ребята из комьюнити очень любят разбираться в коде полученного патча. Поэтому, ни в коем случае не делайте техническую документацию. Wiki, Jira, Mail list discussion — все это полная ерунда. А ерунда не для вас!
Когда вы пришлете патч на [+5000, -3500] с описанием доработки в виде «Factory methods improvements and some other enhancements», все сами во всем разберутся, а заодно поймут, какой вы молодец.
Никогда не прогоняйте тесты
Любые тесты опасны тем, что могут что-нибудь сломать. Если вас все-таки угораздило запустить тесты, не показывайте их результаты. Ошибки в результатах могут привести к отклонению патча! И уже точно никогда не пишите новых тестов. Run All будет дольше, потребление процессорного времени вырастет. Да и вообще, хорошие разработчики пишут код без багов — любой опытный коллега посмотрит в ваши патчи и поймет, что ошибок там нет.
Никогда не читайте How-To
Приходя в opensource-проект, никогда не читайте никаких How-To. Их пишут для дураков, правильно?
Оформляйте код по-своему. Иначе как все поймут, что вы хороший разработчик и разносторонняя творческая личность с развитым чувством прекрасного?
Патчи присылайте так, как удобно вам. Например, проект завязан на GitHub инфраструктуру — присылайте так, как шлют в linux kernel, прямо в письме. Можно даже не в аттаче, а прямо в тексте. Ведь Линус Торвальдс плохого не посоветует! А если в проекте принято по-другому, то это проблемы проекта.
Не стесняйтесь усложнять Public API
Разрабатывая новый Public API, постарайтесь сделать его максимально абстрактным и сложным. На любые вопросы пользователей о неочевидном поведении API всегда можно ответить: «Разве ты не читал мануал? Страница 42, там все понятно написано! Прочитай еще раз!». В серьезных проектах все должно быть сложно. У нас же серьезный проект?
Не советуйтесь и не рассказывайте о проблемах
Ни с кем не советуйтесь и никому не рассказывайте о проблемах, с которыми столкнулись в разработке. Гораздо веселее разбираться во всем одному. Да и вообще, у хороших разработчиков всегда все получается с первого раза, у них проблем не бывает. Если другие узнают о ваших проблемах, то станут умнее и будут писать код быстрее вас. Такого допускать нельзя. Да и вообще о своих проблемах говорить не принято.
Об ограничениях финального решения тоже упоминать не стоит. Ведь решение могут попросить доработать. Кому интересно слушать про ограничения? Когда пользователь начнет внедрять ваш продукт в прод и столкнется с ограничениями, ему будет интереснее и веселее. Он придет к вам и попросит допилить. А до этого момента ни в коем случае не рассказывайте ему об ограничениях — вдруг он решит ничего не внедрять?
Действительно хороший ревьювер все найдет и сам спросит у вас подробности. Ни в коем случае не рассказывайте ему ни о чем.
Если возникают вопросы, пишите на dev-лист
Этот совет дополняет предыдущий. Лучше всего не только никому ничего не рассказывать, но и задавать любые вопросы прежде всего на dev-лист.
Вот примеры вопросов, на которые все любят отвечать. Если пишите какой-то тест, обязательно спросите, «Нужна ли проверка на null этой коллекции?». Разбираться самостоятельно не надо, всегда же можно спросить на dev-листе. Ведь там много людей, которые только и ждут, когда зададут такой вопрос. Они будут стремиться быстрее на него ответить.
«Как мне сделать задачу?» — еще один прекрасный вопрос. Ни в коем случае не указывайте никаких подробностей: ID-шника задачи будет достаточно. Все кому надо, сами посмотрят.
«Какой фреймворк заюзать?» — тоже очень хороший вопрос. На него всегда интересно отвечать, да и подискутировать можно.
Исправляйте все проблемы проекта в одном pull request
Это мой любимый совет. Исправляйте все проблемы проекта в одном pull request, особенно если работаете в энтерпрайзе. До вас в проекте были одни дураки, они писали код плохо. А вы пишете хорошо. Соответственно вы просто обязаны:
- зарефакторить весь существующий код, который вам непонятен;
- переименовать все, на ваш взгляд, неправильно названные переменные;
- исправить все существующие javadoc комментарии.
И вообще можно взять и вынести какие-нибудь классы, фабрики и т.п., сделать другие преобразования, чтобы код был лучше. Особенно эффектно это будет смотреться в областях, которые не относятся к вашей задаче. Так вы сможете полнее раскрыть свой потенциал. Во Славу ООП! Аминь!
Запрашивайте code review заранее
Когда вы делаете задачу, а потом отправляете запрос на code review, процесс может затянуться. Все эксперты обычно заняты. Воспользуйтесь хитростью: когда ваша задача, как вам кажется, скоро закончится, запросите ревью заранее. Ведь сразу смотреть не будут — пока до вас руки дойдут, как раз можно все доделать.
Ну, а если у ревьювера неожиданно есть время, пока еще задача не доделана, то ему не повезло. Объясните ситуацию, скажите, что завтра все будет готово. Таким образом вы ускорите процесс (по крайней мере на ревью) и добьетесь merge быстрее.
Надоела задача — бросайте
У всех, кто работает в open source, очень много времени. Не надо ничего доделывать. Прошли code review, получили комментарии. А зачем что-то править и доводить до merge? Берите следующую задачку и делайте. Это путь к успеху! У ревьювера много времени, посмотрит и следующий патч без результата!
Ревьюер — ваш враг
А как еще назвать человека, который стоит между вами и коммитом в master? Критика кода — это критика лично вас! Кем он себя возомнил? При личном общении «бомбите» как можно больше! Иначе как ревьювер узнает, что вам не все равно? Это основное правило разработки!
Этот совет рекомендую применять и в повседневной разработке. Это помогает достичь результата и сделать правильно.
Вредные советы для ревьювера
Не автоматизируйте рутинные проверки
Если вы достигли в проекте того уровня, когда вам уже присылают патчи на ревью, ни в коем случае не автоматизируйте рутинные проверки! Гораздо увлекательней потратить пару циклов ревью на устранение проблем и обсуждение:
- форматирования кода;
- именования переменных;
- проверки, а те ли переменные помечены final, которые могут быть им помечены;
- … и всего остального.
Рутинные проверки ни в коем случае автоматизировать не надо. Да и тесты гонять ни к чему.
Никогда не раскрывайте все правила игры до самого конца
Чтобы быть впереди, всегда нужно держать пару тузов в рукаве. Не стоит никому рассказывать о необходимости обратной совместимости. Лучше перед самым коммитом сказать: «Здесь по нашим правилам нужно обеспечить backward compatibility. Пожалуйста, поправь». Это будет особенно эффектно, если вы уже раз пять поревьювили. На шестой еще можно сказать: «Я не эксперт, так что нужно дождаться ревью мистера Х, который посмотрит еще раз».
Такие замечания, особенно на финальном этапе ревью, всегда добавляют контрибьютерам мотивации.
Эмоции, авторитеты и никаких благодарностей
Этот совет пересекается с последним советом контрибьютера. Пишите комментарии к pull request максимально эмоционально. Если где-то не проверено на null или не так названа переменная, добавьте к своим замечаниям страсти. Пусть видят, что вам не все равно.
Если возник какой-то спор, ни в коем случае не приводите технических аргументов. С техническими аргументами может оказаться, что вы не правы. Ссылайтесь на авторитеты — это самый лучший аргумент в споре, вы всегда будете побеждать.
Если кто-то все-таки прошел ваше ревью, ни в коем случае нельзя говорить «спасибо». Все же хотят коммитить в open source! Ни в коем случае не надо благодарить, они и так придут еще раз!
А теперь серьезно: о чем нужно подумать при подготовке и проведении code review?
Я надеюсь, все поняли, как надо проводить и проходить ревью? В каждом из этих советов кроме сарказма есть боль, которая частенько встречается на практике.
Выступлю капитаном Очевидность и расскажу, о чем действительно нужно подумать при подготовке и проведении code review. Соображения далее применимы как к тому, который разрабатывает фичу, так и к тому, который ее будет ревьювить. Все-таки это две стороны одной медали.
Консенсус в общении, во-первых, достижим, во-вторых, необходим, чтобы проект двигался вперед. В настоящее время мало продуктов можно разрабатывать в одиночку. Обычно это командная работа.
Используйте здравый смысл
Это самый главный совет. Используйте здравый смысл, он рулит. Мне кажется, этот совет применим ко всем жизненным ситуациям. Если вы что-то делаете, подумайте, отвечает ли это простому правилу здравого смысла?
Предположите…
Это уже про культуру. Я был в нескольких больших open source community. Не знаю, является ли это частью российского менталитета, но часто человек, который может быть воспринят как начальник, одновременно воспринимается как человек, который попал на свое место случайно. Считается, что он хочет вам плохого, по умолчанию появляются сомнения в его профессионализме. Поэтому, очень важно в любой работе предположить хотя бы на секунду, что:
- Ревьювер (контрибьютер, ваш начальник или коллега) вам не враг. Он не хочет вам плохого. Это простое предположение, но достаточно часто оно не делается. Я советую его все-таки делать.
- Человек, который пишет вам замечания, тоже хороший инженер. Вы, понятно, хороший, такую фичу сделали. Но на свете есть много других хороших инженеров. И тот, кто прислал вам замечания, тоже к ним относится.
- Этот человек тоже хочет, чтобы ваша задача была решена.
Когда он что-то просит, у него есть какая-то мотивация. Он это делает не просто так. Особенно если этот человек не первый день в проекте. Наверняка есть какая-то причина. Об этой причине можно спросить: Почему нужно делать именно так? Почему здесь нужна обратная совместимость? Если задавать простые вопросы спокойно, аргументированно и слушать ответы, можно достичь большего.
Какую ценность доработка принесет продукту?
Ревью проходят не только готовые патчи, но и улучшения проекта, исправления. По сути, code review начинается в тот момент, когда вы только обсуждаете свою доработку. В этот момент спросите себя: какую ценность доработка принесёт продукту?
- Это будет новая фича?
- Это улучшение существующей?
- Расширение существующей фичи?
- Это будет рефакторинг кода? В этом нет ничего плохого. Некоторые относятся к рефакторингу критично, но он необходим. И нужно осознавать, что вы делаете именно его, а не новую фичу или еще что-то.
- Это ускорение какого-то процесса, улучшение производительности?
- Это исправление ошибки?
Есть и другие варианты. В любом случае, начиная что-то разрабатывать, решать задачу, вы должны понимать, какую ценность добавите в проект.
Почему доработка (фича) именно такая?
Есть ряд полезных вопросов, которые стоит задавать.
Зачем делать фичу? Зачем эта фича нужна? Ответ на этот вопрос важен.
Где начало работы? В моей практике бывало, что меня просят переделать некое приложение. Есть приложение А, нужно сделать приложение Б, которое делает практически то же самое с небольшими изменениями. Я начинаю делать работу и оказывается, что А в принципе не работает. По факту оно используется где-то на продакшене с помощью человеко-машинного интерфейса — то есть кто-то сидит и постоянно перезапускает программу, исправляя null pointer exception буквально в прямом эфире. Где начало работы? В данном случае оно будет будет в исправлении программы А, чтобы она стабильно работала, потом в написании программы Б.
Где полное окончание работы? Как выполненная работа должна выглядеть в идеале? Важно сформулировать с самого начала, куда вы все-таки идете.
Где окончание текущего этапа? Понятно, что слона сразу не съесть и работу лучше разбивать по этапам. Важно понимать, где окончание текущего этапа. Часто проекты раздуваются и не заканчиваются просто из-за того, что scope проекта становится бесконечно большим.
Почему фича разбита именно на такие этапы? Это про MVP и все такое. Пожалуйста, задумывайтесь и об этом тоже.
Теперь о Public API
О свойствах Public API есть множество статей. Прочитайте перед тем, как его реализовывать. В качестве хорошего примера могу привести фреймворк JQuery, Spring в Java. Существуют и отрицательные примеры. Кто не первый год программирует на Java, наверное помнит просто ужасный с точки зрения Public API EJB 2.1. Может функциональность там и неплохая, но если Public API плохой, ни у кого не получится убедить пользователей применять продукт.
Public API — это не только инструмент для сторонних пользователей. Это и API внутренних компонентов, которые вам самим между собой переиспользовать. Важные свойства Public API:
- Простота.
- Очевидность.
- Правильные умолчания. Стоит подумать о них, если, например, на тестовом окружении вы создаете 500 потоков, как на продакшене. Или наоборот, в продакшене по умолчанию 3 потока, как на тестовом окружении.
- Понятные сообщения об ошибках. Это бич очень многих продуктов. Когда что-то валится изнутри системы, непонятно, что сделано неправильно. Вчера работало, сегодня null pointer exception. Что именно вы сделали не так и как это исправить, из сообщения об ошибке не понятно.
- Сложно допустить ошибку. На этот счет есть очень много рекомендаций. Compile time проверки всегда лучше чем проверки runtime и т.п.
- Понятные и достаточные логи. Это особенно важно, когда вы пишете код, который будет переиспользоваться и деплоиться куда-то на сервер.
- Возможность мониторинга. Нужно понимать, что логи и мониторинг — это тоже часть вашего Public API. При разборе ошибок пользователи будут смотреть, как ведут себя метрики, которые вы в мониторинг выплевываете.
Изменения в подсистемах
Когда вы приходите на code review, важно иметь в голове полный список систем и подсистем большого продукта, где у вас происходят изменения. В энтерпрайз-проектах может быть непонятно: то ли это схема базы данных, то ли контроллер, то ли представление, какая-то система отчетов, выгрузки, загрузки и т.п.
В работе с коробочным продуктом важно задать себе вопрос: как изменения влияют на существующие процессы в системе? Есть ли backward compatibility? Влияет ли это на performance? Если влияет, то на производительность чего? Как это влияет на user experience?
Стандартные процессы системы
В каждой бизнес-системе есть стандартные процессы: старт, установка, какой-то список операций. Как они будут протекать теперь? Перед code review важно это понять. Вы должны пройти именно по коду, который эти процессы имплементирует. В случае Ignite это:
- Node Start/Stop (server/client, coordinator/regular) — старт, остановка ноды, старт серверной или клиентской ноды, старт координатора или обычной ноды
- Node Join — с точки зрения новой ноды/координатора/серверной/клиентской
- Cluster activation (& deactivation)
- Смена координатора
- Создание / Удаление кэша
- Persistence / BLT / MVCC
Понятно, что набор этих процессов достаточно большой. Важно понимать, что такие процессы есть и как они изменяются.
Corner cases
У вас в бизнес-приложении может случиться восстановление после сбоев, первичная инициализация системы, остановка узла, рестарт, обновление вашего приложения и тому подобные вещи. В случае Ignite у нас есть следующие corner cases:
- смена координатора;
- падение ноды;
- сетевые проблемы — когда сетевые сообщения не доходят;
- битые файлы.
Эти вещи мы должны проверить и проконтролировать, что все в порядке.
Хорошие свойства кода
Вот мы и дошли до кода. Советую не лениться и добиваться в нем:
- простоты,
- расширяемости,
- тестируемости,
- надежности,
- высокой скорости работы.
Concurrency
В Java есть свои особенности при написании сoncurrency кода. Если у вас бизнес-система, и там достаточно мало concurrency — вам не нужно эти особенности учитывать. Тем не менее обычно какие-нибудь синхронизации проходят через БД. В таких вещах как Ignite это устроено чуть сложнее. И тут важна не только функциональность кода, но и его свойства:
- Насколько сложно понять вашу Concurrency модель?
- Shared data structure — как гарантируется их целостность?
- Locks — какие и для чего?
- Threads — какие pools используются? Почему?
- Гарантии видимости изменений — чем обеспечиваются?
Эти вопросы нужно задать перед тем, как ревьювить concurrency код. Понятно, что этот список можно очень долго продолжать.
Тесты производительности. Benchmarks
Если вы дорабатываете какую-то систему, у нее есть клиенты, инсталляции, то она, очевидно, работает с некоей производительностью. В современном мире невозможно увеличивать аппаратные мощности бесконечно. Нужны тесты и контроль производительности. При проведении код-ревью важно понять:
- Нужно ли вообще проводить тестирование производительности? Может это какая-то доработка, которой тесты производительности не нужны?
- Если нужно, то какое именно? Есть много методик и способов измерений и т.п.
- Где-как-что нужно измерять?
- Какие бенчмарки показательны? Количество нод? Железа? Конфигурация Ignite? Характер нагрузки?
Итого
В целом, code review — очень полезная практика. Надеюсь, все разработчики (в том числе энтерпрайз-продуктов) уже внедрили её у себя. Если нет, пожалуйста, внедряйте как можно быстрее. Буду рад обсудить с вами практики code review в комментариях.
Видеозапись лекции:
Презентация доступна здесь.
