Что делать, если код, представленный для проверки кода, кажется слишком сложным?

115

Код трудно следовать, но он (в основном) работает хорошо, по крайней мере, при поверхностном тестировании. Здесь и там могут быть небольшие ошибки, но по коду кода очень трудно определить, являются ли они симптомами более глубоких проблем или простых исправлений. Проверка общей правильности вручную с помощью проверки кода, однако, очень трудна и трудоемка, если вообще возможна.

Каков наилучший курс действий в этой ситуации? Настаивать на переоценке? Частичная переработка? Рефакторинг первым? Исправить только ошибки и принять технический долг ? Сделайте оценку риска по этим вариантам и затем решите? Что-то другое?

Брэд Томас
источник
4
Появляется или есть? Быстрое измерение исходных файлов подтвердит или опровергнет ваши подозрения. После измерения просто вызовите номера измерений при просмотре кода и предложите рефакторинг, чтобы уменьшить число сложностей.
Джон Рейнор
4
Пожалуйста, определите «слишком сложно». Является ли код слишком сложным, потому что он использует хорошо известные шаблоны проектирования, которые просто незнакомы вашей команде, или потому что он не использует шаблоны, хорошо известные вашей команде? Точные причины для того, чтобы судить код «слишком сложный», необходимы для правильной оценки того, как двигаться дальше. Простое и «слишком сложное» утверждение в области знаний, столь же глубокое и сложное, как обзор кода, предлагает мне охоту на ведьм разработчика.
Питер Гиркенс
7
@PieterGeerkens Или, может быть, это слишком сложно, потому что это решает сложную проблему?
Кейси

Ответы:

251

Если он не может быть рецензирован, он не может пройти рецензирование.

Вы должны понимать, что обзор кода не для поиска ошибок. Вот для чего QA. Проверка кода должна обеспечить возможность дальнейшего обслуживания кода. Если вы не можете даже следовать коду сейчас, как вы можете через шесть месяцев, когда вам поручено делать улучшения функций и / или исправлять ошибки? Обнаружение ошибок прямо сейчас является лишь побочным преимуществом.

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

Кевин Фе
источник
49
Это очень многое. Помните, что не только вы, но и писатель будет читать этот код. Через 10 лет он также будет случайным стажером, поэтому вы должны убедиться, что у него есть шанс понять, что происходит.
Дэвид Гринберг
2
хороший ответ это зависит от цели «проверки кода». удобочитаемость - это одно, а структура - другое, но они очень тесно связаны. fwiw Я работаю с открытым исходным кодом, написанным корпусом MAJOR, и он почти не читается, потому что имена var и fn так запутаны.
19
@DavidGrinberg Для всех практических целей «вы через шесть месяцев» - это совершенно другой человек.
Хрилис - на забастовке-
2
Положите код на некоторое время (достаточно долго, чтобы он не запомнил все). Попросите оригинального кодера просмотреть его. Посмотрите, понимает ли он это.
Нельсон
4
Я не согласен с тем, что обзор кода "не" для поиска ошибок. Он часто находит ошибки, и это очень мощный и полезный аспект анализа кода. А еще лучше, это помогает найти способы полностью избежать ошибок в будущем коде. Дело, возможно, преувеличено, и должно быть, это не только для поиска ошибок!
Коди Грей
45

Все, что вы упомянули, совершенно уместно указать в обзоре кода.

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

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

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

Томас Оуэнс
источник
30

Проверка общей правильности вручную с помощью проверки кода, однако, очень трудна и трудоемка, если вообще возможна.

Это далеко не точка пересмотра кода. Чтобы подумать о проверке кода, нужно представить, что в коде есть ошибка, и вы должны ее исправить. С этим мышлением просмотрите код (особенно комментарии) и спросите себя: «Легко ли понять общую картину происходящего, чтобы я мог сузить проблему?» Если это так, это пропуск. Иначе это провал. По крайней мере, требуется больше документации, или, возможно, потребуется рефакторинг, чтобы сделать код достаточно понятным.

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

DepressedDaniel
источник
4
Отличный комментарий! «Большая часть кода сосет так много , что она легко может быть переработан в 10 раз подряд, получая более читаемый каждый раз , когда » Мальчик, я был виновен , что делать :)
Дин Рэдклиффа
1
«Большая часть кода отстой настолько, что его можно легко реорганизовать 10 раз подряд, делая каждый раз более читабельным». Действительно, так оно и есть в реальном мире.
Питер Мортенсен
@PeterMortensen Это действительно правда, что вы найдете много этого в реальном мире. Но никто не заинтересован в том, чтобы код был написан таким образом. Я думаю, что есть две причины, почему это так. Обучение, которое получают разработчики, вкладывает очень мало усилий в обучение написанию читабельного кода. А в некоторых компаниях это воспринимается как пустая трата времени: «Если разработчик уже написал рабочий код, почему нас должно волновать, читается он или нет? Просто отправьте его».
kasperd
15

Проверка общей правильности вручную с помощью проверки кода, однако, очень трудна и трудоемка, если вообще возможна.

Много лет назад моя работа заключалась в том, чтобы делать именно это, оценивая домашние задания студентов. И в то время как многие из них доставляли некое приемлемое качество с ошибками, кое-где выделялись. Оба всегда отправляли код без ошибок. Один представленный код, который я мог читать сверху и снизу на высокой скорости и пометить как 100% правильный с нулевым усилием. Другой представил код, который был один WTF за другим, но каким-то образом удалось избежать каких-либо ошибок. Абсолютная боль, чтобы отметить.

Сегодня у второго будет отклонен его код в обзоре кода. Если проверка правильности очень трудна и занимает много времени, то это проблема с кодом. Приличный программист выяснить , как решить проблему (требуется время X) и прежде чем дать обзор кода реорганизовать его так , чтобы он не просто делать свою работу, но , очевидно , делает работу. Это занимает намного меньше времени и экономит много времени в будущем. Зачастую путем выявления ошибок еще до того, как они перейдут на этап проверки кода. Затем, сделав обзор кода намного быстрее. И все время в будущем, делая код легче адаптировать.

В другом ответе говорилось, что код некоторых людей может подвергаться рефакторингу 10 раз, становясь более читаемым с каждым разом. Это просто грустно. Это разработчик, который должен искать другую работу.

gnasher729
источник
Мне требуется гораздо меньше времени для рефакторинга моего кода 10 раз, а затем мне нужно написать первую версию кода. Если кто-то еще знает, что я сделал этот рефакторинг, я потерпел неудачу.
Ян
6

Этот старый код был немного изменен? (100 строк кода, измененных в кодовой базе 10000 строк, все еще являются небольшими изменениями) Иногда существуют временные ограничения, и разработчики вынуждены оставаться в старой и неудобной среде, просто потому, что полное переписывание может занять еще больше времени и выход из бюджета , + обычно присутствует риск, который при неправильной оценке может стоить миллионы долларов. Если это старый код, в большинстве случаев вам придется жить с ним. Если вы сами этого не понимаете, поговорите с ними и послушайте, что они говорят, постарайтесь понять. Помните, что вам может быть трудно следовать, но совершенно нормально для других людей. Примите их сторону, посмотрите на это с их конца.

Это новый код ? В зависимости от временных ограничений, вы должны пропагандировать как можно больше рефакторинга. Можно ли тратить больше времени на проверку кода, если это необходимо. Вы не должны ставить время на 15 минут, понять идею и двигаться дальше. Если автор потратил неделю на то, чтобы написать что-то, можно потратить 4-8 часов на его рецензирование. Ваша цель здесь, чтобы помочь им рефакторинг. Вы не просто возвращаете код с надписью «рефакторинг. Сейчас». Посмотрите, какие методы можно разбить, попробуйте придумать идеи для введения новых классов и т. Д.

Neolisk
источник
2
Вы не просто возвращаете код с надписью «рефакторинг. Сейчас» - почему? Я получил такие комментарии к обзору по крайней мере один раз, и в прошлый раз я помню, что это оказалось полезным и правильным. Я должен был переписать большой кусок кода с нуля, и это было правильно, потому что, оглядываясь назад, я сам видел, что старый код был неразрешимым беспорядком. Рецензент был достаточно квалифицирован, чтобы заметить это (а я, очевидно, не был)
комнат
4
@gnat: Во-первых, потому что это грубо. Вы выглядите лучше, когда объясняете, что не так с кодом, и прилагаете усилия, чтобы помочь другому человеку улучшить его. В большой компании, если вы сделаете это иначе, вы можете быстро выйти за дверь. Особенно если вы просматриваете код более старшего человека.
Neolisk
этот случай я говорил выше, это было в значительной созданной компании , что только что произошло , чтобы быть достаточно осторожным , не выходя из двери своих наиболее квалифицированных разработчиков, по крайней мере , не на основании непосредственно обмена их техническую экспертизу , когда просят
комара
1
@gnat: Подход «рефакторинг. сейчас» может работать неэффективно, т. е. когда старший разработчик с более чем 10-летним опытом работы говорит рефакторинг младшему разработчику, который был нанят 1 месяц назад без опыта или в аналогичной ситуации. Вверх - у вас могут быть проблемы. Поскольку вы, возможно, не знаете, какой опыт имеет другой разработчик, можно считать его уважением по умолчанию. Тебе точно не повредит.
Neolisk
1
@Neolisk: опытный разработчик, который должен был писать код под давлением времени и знает, что он недостаточно хорош, может быть только рад, если вы отклоните код, предоставив ему время и повод для его улучшения. PHB, решивший, что он достаточно хорош, делает разработчика несчастным; рецензент, решивший, что он недостаточно хорош, делает его счастливым.
gnasher729
2

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

Распространенная подсказка в том, что патч огромный, но его описание крошечное: «Реализуйте $ FOO».

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

Например, первые патчи могут содержать чисто механические рефакторинги, которые не вносят никаких функциональных изменений, а затем финальные патчи могут сфокусироваться на фактической реализации и тестировании $ FOO с меньшим количеством отвлекающих факторов и красных селедок.

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

Что касается тактичного выполнения, я обычно называю это своей проблемой, а затем обращаюсь за помощью к автору: «У меня проблемы с отслеживанием всего происходящего здесь. Не могли бы вы разбить этот патч на более мелкие шаги, чтобы помочь мне понять, как все это подходит все вместе?" Иногда необходимо сделать конкретные предложения для небольших шагов.

Такой большой патч, как "Implement $ FOO", превращается в серию патчей вроде:

  1. Представьте новую версию Frobnicate, которая использует пару итераторов, потому что мне нужно будет вызывать ее с последовательностями, отличными от vector, для реализации $ FOO.
  2. Переключите все существующие абоненты Frobnicate, чтобы использовать новую версию.
  3. Удалить старый Frobnicate.
  4. Frobnicate делал слишком много. Внесите шаг повторения в свой метод и добавьте тесты для этого.
  5. Представьте Zerzify, с тестами. Пока не используется, но мне нужно за $ FOO.
  6. Реализуйте $ FOO в терминах Zerzify и нового Frobnicate.

Обратите внимание, что шаги 1-5 не вносят никаких функциональных изменений в продукт. Их тривиально проверить, в том числе убедиться, что у вас есть все нужные тесты. Даже если шаг 6 все еще «сложен», по крайней мере, он сосредоточен на $ FOO. И журнал, естественно, дает вам гораздо лучшее представление о том, как был реализован $ FOO (и почему был изменен Frobnicate).

Адриан Маккарти
источник
Один из подходов, если используется Git, - это составить запрос на получение нескольких коммитов. Каждый коммит настолько атомарен и самодостаточен, насколько это возможно, и имеет свое собственное описание. Затем добавьте полезную заметку в тело PR, чтобы каждое изменение можно было просмотреть вручную. Как правило, именно так я справляюсь с очень большими PR, такими как глобальные рефакторинг или большие неопровержимые изменения инструментов.
Джимми Брек-Макки
1

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

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

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

Последний пункт. Как рецензент, вы должны решить, является ли сложность кода следствием существенной или случайной сложности . Существенная сложность связана с частями программного обеспечения, которые на законных основаниях трудно решить. Случайная сложность относится ко всем другим частям кода, который мы пишем, который слишком сложен без причины и может быть легко упрощен.

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

c_maker
источник
0

На что похожи тесты? Они должны быть четкими, простыми и легко читаемыми, в идеале только с одним утверждением. Тесты должны четко документировать предполагаемое поведение и варианты использования кода.

Если это не хорошо проверено, это хорошее место, чтобы начать рецензирование.

Том Сквайрс
источник