Код трудно следовать, но он (в основном) работает хорошо, по крайней мере, при поверхностном тестировании. Здесь и там могут быть небольшие ошибки, но по коду кода очень трудно определить, являются ли они симптомами более глубоких проблем или простых исправлений. Проверка общей правильности вручную с помощью проверки кода, однако, очень трудна и трудоемка, если вообще возможна.
Каков наилучший курс действий в этой ситуации? Настаивать на переоценке? Частичная переработка? Рефакторинг первым? Исправить только ошибки и принять технический долг ? Сделайте оценку риска по этим вариантам и затем решите? Что-то другое?
code-reviews
Брэд Томас
источник
источник
Ответы:
Если он не может быть рецензирован, он не может пройти рецензирование.
Вы должны понимать, что обзор кода не для поиска ошибок. Вот для чего QA. Проверка кода должна обеспечить возможность дальнейшего обслуживания кода. Если вы не можете даже следовать коду сейчас, как вы можете через шесть месяцев, когда вам поручено делать улучшения функций и / или исправлять ошибки? Обнаружение ошибок прямо сейчас является лишь побочным преимуществом.
Если это слишком сложно, это нарушает тонну твердых принципов . Рефакторинг, рефакторинг, рефакторинг. Разбейте его на правильно названные функции, которые делают намного меньше, проще. Вы можете почистить его, и ваши тесты убедятся, что он продолжает работать правильно. У вас есть тестовые случаи, верно? Если нет, вы должны начать добавлять их.
источник
Все, что вы упомянули, совершенно уместно указать в обзоре кода.
Когда я получаю обзор кода, я проверяю тесты. Если тесты не дают достаточного покрытия, на это следует обратить внимание. Тесты должны быть полезны, чтобы гарантировать, что код работает так, как задумано, и будет продолжать работать так, как задумано, в изменениях. Фактически, это одна из первых вещей, которую я ищу в обзоре кода. Если вы не доказали, что ваш код отвечает требованиям, я не хочу тратить свое время на его изучение.
Если есть достаточное количество тестов для кода, если код сложный или трудный для отслеживания, это также то, на что должны обратить внимание люди. Инструменты статического анализа могут указывать на некоторые показатели сложности и отмечать чрезмерно сложные методы, а также обнаруживать потенциальные недостатки в коде (и их следует запускать перед проверкой человеческого кода). Но код читается и поддерживается людьми, и его нужно сначала написать для удобства обслуживания. Только если есть причина использовать менее поддерживаемый код, он должен быть написан таким образом. Если вам нужен сложный или не интуитивно понятный код, он должен быть задокументирован (желательно в коде), почему код такой, и иметь полезные комментарии для будущих разработчиков, чтобы понять, почему и что делает код.
В идеале, следует отклонять обзоры кода, которые не имеют соответствующих тестов или имеют слишком сложный код без уважительной причины. Для продвижения вперед могут быть деловые причины, и для этого вам необходимо оценить риски. Если у вас возникнут технические проблемы с кодом, немедленно добавьте в систему отслеживания ошибок билеты с некоторыми подробностями того, что нужно изменить, и некоторыми предложениями по их изменению.
источник
Это далеко не точка пересмотра кода. Чтобы подумать о проверке кода, нужно представить, что в коде есть ошибка, и вы должны ее исправить. С этим мышлением просмотрите код (особенно комментарии) и спросите себя: «Легко ли понять общую картину происходящего, чтобы я мог сузить проблему?» Если это так, это пропуск. Иначе это провал. По крайней мере, требуется больше документации, или, возможно, потребуется рефакторинг, чтобы сделать код достаточно понятным.
Важно не быть перфекционистом в этом, если вы не уверены, что это то, что ваш работодатель после. Большая часть кода отстой настолько, что его можно легко реорганизовать 10 раз подряд, и каждый раз он становится более читабельным. Но ваш работодатель, вероятно, не хочет платить, чтобы иметь самый читаемый код в мире.
источник
Много лет назад моя работа заключалась в том, чтобы делать именно это, оценивая домашние задания студентов. И в то время как многие из них доставляли некое приемлемое качество с ошибками, кое-где выделялись. Оба всегда отправляли код без ошибок. Один представленный код, который я мог читать сверху и снизу на высокой скорости и пометить как 100% правильный с нулевым усилием. Другой представил код, который был один WTF за другим, но каким-то образом удалось избежать каких-либо ошибок. Абсолютная боль, чтобы отметить.
Сегодня у второго будет отклонен его код в обзоре кода. Если проверка правильности очень трудна и занимает много времени, то это проблема с кодом. Приличный программист выяснить , как решить проблему (требуется время X) и прежде чем дать обзор кода реорганизовать его так , чтобы он не просто делать свою работу, но , очевидно , делает работу. Это занимает намного меньше времени и экономит много времени в будущем. Зачастую путем выявления ошибок еще до того, как они перейдут на этап проверки кода. Затем, сделав обзор кода намного быстрее. И все время в будущем, делая код легче адаптировать.
В другом ответе говорилось, что код некоторых людей может подвергаться рефакторингу 10 раз, становясь более читаемым с каждым разом. Это просто грустно. Это разработчик, который должен искать другую работу.
источник
Этот старый код был немного изменен? (100 строк кода, измененных в кодовой базе 10000 строк, все еще являются небольшими изменениями) Иногда существуют временные ограничения, и разработчики вынуждены оставаться в старой и неудобной среде, просто потому, что полное переписывание может занять еще больше времени и выход из бюджета , + обычно присутствует риск, который при неправильной оценке может стоить миллионы долларов. Если это старый код, в большинстве случаев вам придется жить с ним. Если вы сами этого не понимаете, поговорите с ними и послушайте, что они говорят, постарайтесь понять. Помните, что вам может быть трудно следовать, но совершенно нормально для других людей. Примите их сторону, посмотрите на это с их конца.
Это новый код ? В зависимости от временных ограничений, вы должны пропагандировать как можно больше рефакторинга. Можно ли тратить больше времени на проверку кода, если это необходимо. Вы не должны ставить время на 15 минут, понять идею и двигаться дальше. Если автор потратил неделю на то, чтобы написать что-то, можно потратить 4-8 часов на его рецензирование. Ваша цель здесь, чтобы помочь им рефакторинг. Вы не просто возвращаете код с надписью «рефакторинг. Сейчас». Посмотрите, какие методы можно разбить, попробуйте придумать идеи для введения новых классов и т. Д.
источник
Часто «сложные» патчи / списки изменений - это те, которые делают много разных вещей одновременно. Есть новый код, удаленный код, переработанный код, перемещенный код, расширенные тесты; это мешает видеть общую картину.
Распространенная подсказка в том, что патч огромный, но его описание крошечное: «Реализуйте $ FOO».
Разумный способ справиться с таким патчем - попросить разбить его на ряд более мелких, автономных частей. Точно так же, как принцип единственной ответственности гласит, что функция должна выполнять только одну вещь, патч должен фокусироваться только на одной вещи.
Например, первые патчи могут содержать чисто механические рефакторинги, которые не вносят никаких функциональных изменений, а затем финальные патчи могут сфокусироваться на фактической реализации и тестировании $ FOO с меньшим количеством отвлекающих факторов и красных селедок.
Для функциональности, которая требует большого количества нового кода, новый код часто может быть представлен в тестируемых блоках, которые не изменяют поведение продукта до тех пор, пока последний патч в серии не вызовет новый код (изменение флага).
Что касается тактичного выполнения, я обычно называю это своей проблемой, а затем обращаюсь за помощью к автору: «У меня проблемы с отслеживанием всего происходящего здесь. Не могли бы вы разбить этот патч на более мелкие шаги, чтобы помочь мне понять, как все это подходит все вместе?" Иногда необходимо сделать конкретные предложения для небольших шагов.
Такой большой патч, как "Implement $ FOO", превращается в серию патчей вроде:
Обратите внимание, что шаги 1-5 не вносят никаких функциональных изменений в продукт. Их тривиально проверить, в том числе убедиться, что у вас есть все нужные тесты. Даже если шаг 6 все еще «сложен», по крайней мере, он сосредоточен на $ FOO. И журнал, естественно, дает вам гораздо лучшее представление о том, как был реализован $ FOO (и почему был изменен Frobnicate).
источник
Как отмечали другие, обзор кода не предназначен для поиска ошибок. Если вы обнаруживаете ошибки во время проверки кода, это, вероятно, означает, что у вас недостаточно автоматизированного тестирования (например, модульные / интеграционные тесты). Если нет достаточного охвата, чтобы убедить меня в том, что код выполняет то, что предполагалось , я обычно запрашиваю дополнительные тесты и указываю, какие тестовые примеры я ищу, и обычно не допускаю код в кодовую базу, которая не имеет достаточного покрытия ,
Если архитектура высокого уровня слишком сложна или не имеет смысла, я обычно созываю встречу с парой членов команды, чтобы поговорить об этом. Иногда сложно повторять плохую архитектуру. Если разработчик был новичком, я обычно проверял, что они думают раньше времени, вместо того, чтобы реагировать на плохой запрос. Обычно это справедливо даже для более опытных разработчиков, если у проблемы нет очевидного решения, которое, скорее всего, будет выбрано.
Если сложность изолирована от уровня метода, который обычно может быть исправлен итеративно и с хорошими автоматизированными тестами.
Последний пункт. Как рецензент, вы должны решить, является ли сложность кода следствием существенной или случайной сложности . Существенная сложность связана с частями программного обеспечения, которые на законных основаниях трудно решить. Случайная сложность относится ко всем другим частям кода, который мы пишем, который слишком сложен без причины и может быть легко упрощен.
Я обычно удостоверяюсь, что код с существенной сложностью действительно так и не может быть упрощен. Я также стремлюсь к большему тестированию и хорошей документации для этих частей. Случайные сложности почти всегда должны быть устранены во время процесса запроса на извлечение, потому что это основная часть кода, с которым мы имеем дело, и может легко вызвать кошмар обслуживания даже в краткосрочной перспективе.
источник
На что похожи тесты? Они должны быть четкими, простыми и легко читаемыми, в идеале только с одним утверждением. Тесты должны четко документировать предполагаемое поведение и варианты использования кода.
Если это не хорошо проверено, это хорошее место, чтобы начать рецензирование.
источник