В обзорах кода должен ли рецензент всегда представлять решение проблем? [закрыто]

93

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

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

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

В общем, как рецензент, хорошо ли говорить «этот код ошибочен, потому что ..., по-другому» или вам нужно придумать конкретное решение?

Фрэнк Пуффер
источник
24
@gnat: Нет, код не слишком сложный. И это только пример. Я обычно спрашиваю, отвечает ли рецензент за представление решения.
Фрэнк
5
нет, я бы сказал, что как рецензент вы не обязаны рассказывать, как его улучшить. Если вы можете описать, что там не так, сделайте это; если нет - просто предоставьте общий комментарий. (Один из самых полезных замечаний обзорного я вспоминаю прием был буквально как «этот класс все общий мусор»)
комар
5
«Стандартный способ решения этой проблемы - разделить класс и использовать наследование». Я очень надеюсь, что вы не просматриваете мой код!
садовник
7
Определить потенциальные проблемы может быть достаточно. Рецензент рассматривает код как пользователя, сопровождающего или дизайнера. Обеспечение другого угла обзора или проблем с определением, которое кодер, возможно, еще не заметил, может помочь кодеру улучшить свою работу. И если вы заметите что-то, что вам понравится, вам тоже не помешает сообщить об этом. Это должно быть не корректирующее упражнение, а просветляющее. Вот почему это называется «экспертная оценка».
Мартин Маат

Ответы:

139

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

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

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

С другой стороны, просто сказать автору «это неправильно. Исправить это» обычно не приводит к позитивной атмосфере в команде. Для позитивной атмосферы полезно хотя бы указать, почему в ваших глазах что-то испорчено, и предложить лучшую альтернативу, если она у вас есть.
Кроме того, если вы просматриваете что-то, что выглядит «неправильно», но у вас нет лучшей альтернативы, то вы также можете оставить комментарий в духе «Этот код / ​​дизайн мне не подходит, но я У меня нет четкой альтернативы. Можем ли мы это обсудить? а затем попытаться получить что-то лучше вместе.

Барт ван Инген Шенау
источник
23
+1 для обсуждения, чтобы прийти к решению вместе - именно так я больше всего учусь у старших программистов, рассматривающих мой код
dj18
19
+1. Оставляя отзыв, лучше всего давать конструктивную критику, когда это возможно.
FrustratedWithFormsDesigner
7
Я согласен с последним, особенно. Совершенно нормально сказать «это решение кажется неправильным, потому что ...» или «я беспокоюсь, что эта часть может быть проблематичной, потому что ...», не давая решения.
Даниэль Т.
1
@dotancohen: «Мы можем обсудить это» задумывалось как вопрос. Лично у меня все равно было бы обсуждение, даже если бы я только научился чему-то самому.
Барт ван Инген Шенау
1
Тонкая опасность в том, что при достаточном обсуждении и взаимодействии реализации это перестает быть обзором и становится парным программированием. Парное программирование - это хорошо, но как только оно будет сделано, вам понадобится третье лицо для проверки, потому что независимость была потеряна.
candied_orange
35

Некоторые хорошие ответы здесь, но я думаю, что один важный момент отсутствует. Это имеет большое значение, чей код вы просматриваете, насколько опытным этот человек и как он или она обрабатывает такие предложения. Если вы хорошо знаете своего товарища по команде и ожидаете, что заметки типа «этот код ошибочен, потому что ... делайте это по-другому», будет достаточно для того, чтобы он или она нашли лучшее решение, тогда такой комментарий может подойти. Но, безусловно, есть люди, которым такого комментария недостаточно, и которым нужно точно сказать, как улучшить свой код. Так что ИМХО, это суждение, которое вы можете сделать только для отдельного случая.

Док Браун
источник
29

В общем, как рецензент, нормально ли говорить «этот код ошибочен, или нет», или вам нужно придумать конкретное решение?

Ни один из двух не является идеальным ИМО. Лучше всего поговорить с автором и решить проблему совместно.

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

guillaume31
источник
"Бюрократический процесс" - это хороший способ выразить это!
frnhr
17

В обзорах кода должен ли рецензент всегда представлять решение проблем?

Нет. Если вы делаете это, вы не рецензент, вы следующий кодер.

В обзорах кода должен ли рецензент никогда не представлять решение проблем?

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

Когда рецензент должен представить решение проблем?

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

candied_orange
источник
8
Не кодируйте в вакууме, потому что это отстой.
Гектометр Когда я предлагаю решение проблемы, оно часто имеет известные мне преимущества, но просто занимает слишком много времени, чтобы дать исчерпывающий список их всех. (Они часто связаны со стабильностью и уверенностью в том, что эта вещь будет работать, пока мы меняем другие вещи вокруг нее.) Поэтому, если вы сделаете что-то еще, что не решит столько проблем, я бы не был полностью счастлив (по крайней мере, нет, если вы не можете сказать мне вескую причину, почему то, что я предложил, не сработало). Как бы вы справились с этим?
jpmc26
1
PS: «Обезьяна кода» часто используется, чтобы описать неквалифицированного, бездумного программиста, который просто делает то, что им говорят, даже если это плохая идея и не имеет хороших дизайнерских чувств. Смотрите городской словарь . Даже Википедия отмечает, что иногда это уничижительно.
jpmc26
@ jpmc26, если вы собираетесь использовать код для общения со мной, я надеюсь, что вы будете использовать код, который показывает, как проблема может быть решена лучше. Кроме того, Code Monkey можно использовать с любовью. Конечно, больше любви, чем английские майоры получают
candied_orange
«Код обезьяна встать получить кофе код обезьяны идти на работу Код обезьяна имеет скучную встречу, с скучный менеджер Роба Роба сказать код обезьяна очень старательная, но его выход вонь ......»
Baldrickk
13

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

Обычно, однако, вы должны по крайней мере определить источник осложнения. «Этот класс слишком сложен, потому что он повсеместно нарушает закон Деметры». «Этот класс смешивает обязанности уровня представления и уровня сохраняемости». Умение выявлять эти причины и кратко их объяснять - часть того, как стать лучшим рецензентом. Вам редко приходится вдаваться в подробности о решениях.

Карл Билефельдт
источник
4
+100 Мое самое частое разочарование в обзорах кода состоит в том, что если бы я знал лучший способ, я, вероятно, написал бы так.
RubberDuck
Мне нравится твое первое предложение. Это заставило меня задуматься: «Достаточно ли хорош этот код?» затем подбрасываете монету, чтобы решить, стоит ли ее улучшать или нет! (Обычно я просто придерживаюсь этого, пока у меня не заканчивается время, но, возможно, я мог бы остановиться, когда это достаточно хорошо вместо этого?)
ИМО «Этот код сложен, потому что он нарушает закон Деметры» - плохой комментарий. «Этот код сложен, потому что X слишком связан с Y, а Z» лучше.
user253751
«Если бы люди знали, как писать код лучше, они обычно делали бы это в первую очередь». Есть исключения. Я разработал этот код, который вроде работает, но когда-нибудь в будущем нас укусит в задницу. Нетехнический менеджер не понимает: «Мне не нравится этот код, и я хочу его улучшить в течение трех дней». Нетехнический менеджер понимает: «Джо просмотрел и отклонил этот код, и мне нужно три дня, чтобы его улучшить».
gnasher729
4

Есть два типа плохих программистов: те, которые не следуют стандартным практикам, и те, которые «только» следуют стандартным практикам.

Когда у меня был ограниченный рабочий контакт / предоставление обратной связи кому-то, я бы не сказал: «Это плохой дизайн». но что-то вроде "Можете ли вы объяснить этот класс для меня?" Вы можете обнаружить, что это хорошее решение, разработчик искренне старался изо всех сил или даже признал, что это плохое решение, но оно достаточно хорошее.

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

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

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

JeffO
источник
1
Но, если это требует объяснения, чтобы быть узнаваемо хорошим дизайном, отсутствуют встроенные комментарии.
Wildcard
1
Иногда правила не имеют исключений, но обычно их нет.
@Wildcard - это зависит от способностей и предпочтений / мнений людей, которые на это смотрят.
Джеффо
1
@Wildcard Я придерживаюсь подхода, согласно которому обратная связь должна быть сформулирована как вопрос, но ответ будет (в конечном итоге) принимать форму комментария или, возможно, изменения кода (например, лучшего именования). Это оставляет открытой возможность для разработчика объяснить свое мышление и обсудить варианты, вместо того, чтобы чувствовать себя требованием или случайно выполнить свою работу за них.
IMSoP
3

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

BagOfSpanners
источник
3

Мое мнение усиливается в отношении отсутствия кода в большинстве случаев по ряду причин:

  • Если одного объяснения недостаточно, они всегда могут попросить образец того, о чем вы думаете.
  • Вы не напрасно тратите свое время, пытаясь ознакомиться с кодом, которого вы давно не касались, просто слегка его изменив, в то время как кто-то другой просто потратил на это свое время.
  • Если они уже знакомы с фрагментом кода, а вы нет, предоставление только обратной связи может привести к созданию лучшего кода, чем вы написали бы. Предоставление кому-либо готового решения часто приводит к тому, что он просто использует его, не рассматривая его дальнейшее улучшение.
  • Всегда предоставление решения граничит с снисходительным. Вы работаете с кем-то, надеюсь, потому что они были достаточно хороши, чтобы их наняли. Если вам удалось узнать, почему что-то является плохой идеей, почему бы им не изучить это, слушая отзывы и делая это самостоятельно?
  • Всегда предоставлять решение просто странно. Представьте, что вы программируете пару за столом: они только что закончили пару строк, которые вы считаете не очень хорошими. Просто скажите им, что вы заметили и почему, или вы берете их клавиатуру и сразу показываете свою версию? Это то, что всегда дает ваше решение другим людям.
  • Вы всегда можете сказать, что вы делаете вместо этого, не тратя времени на то, чтобы действительно это написать. Вы сделали именно это при описании первой проблемы в вопросе.
  • Не раздайте еду, научите ловить рыбу;)

Конечно, есть некоторые случаи, когда вы думаете о какой-то конкретной альтернативе, и ее стоит приложить. Но это действительно редко в моем опыте. (множество обзоров, большие проекты) Автор оригинала всегда может попросить у вас образец, если он понадобится.

Даже тогда, по 3-й причине, при предоставлении образца, возможно, стоит сказать, например, «использование x.foo()сделает это проще», а не полное решение, - и пусть автор напишет его. Это также экономит ваше время.

viraptor
источник
Ваш 5-й пункт заставил меня улыбнуться, я представлял себе «дуэльные клавиатуры», чтобы посмотреть, кто первым сможет найти отличное решение. Кто знал, что парное программирование может походить на эти две гоночные аркадные игры или на полный контакт? « Стив только что жестоко проверил Рона в BSOD, 2 минуты в штрафной ... »
2

Я думаю, что ключом к проверке кода является согласование правил перед проверкой.

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

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

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

Ваш пример наследования (необязательные параметры), возможно, лучше, хотя я видел правила проверки кода, которые запрещают длинные методы и «слишком много» параметров функции. Но обычно они могут быть решены тривиально путем разделения. Это «казалось, что это решение слишком усложняет вещи», где ваша объективность мешает и, возможно, требует обоснования или альтернативного решения.

Ewan
источник
2
«Я думаю, что ключом к проверке кода является согласование правил перед проверкой». Это был бы идеальный случай. На практике мы не можем предполагать, что каждый разработчик знает все правила. Обзоры кода полезны для распространения этих знаний и объяснения правил на практических примерах. Может быть, это одно из величайших преимуществ
Фрэнк
запишите правила в документе о стандартах кодирования и дайте новым разработчикам
Ewan
1
Мы записали стандарты кодирования, и они предоставляются новым разработчикам. Это работает большую часть времени, но иногда бывают неверные интерпретации. В дополнение к записанным стандартам кодирования существуют общие принципы, такие как DRY или SOLID, которые также рассматриваются в обзорах кода. Мы ожидаем от наших разработчиков базовых знаний об этом, а также проводим внутренние тренинги для их улучшения. Это непрерывный процесс, и проверки кода являются его частью.
Фрэнк
0

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

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

Эрик Гидрик
источник
0

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

Письменное общение приводит к огромным потерям времени, а также к обидам и недоразумениям.

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

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

Джон Лоуренс Аспден
источник
это , кажется, не предлагает ничего существенного над точкой сделана и объяснена в ранее 11 ответов
комар