Каковы общие процессы проверки кода и что считается плохим?

16

Моя компания недавно начала делать формализованные проверки кода. Процесс идет следующим образом: вы отправляете в github, запрашиваете запрос на извлечение, код проверяется примерно тремя людьми, а затем, если все прошло, ваш код входит.

Процесс кажется справедливым, однако три человека, которые делают обзоры кода, не кажутся справедливыми. Я замечаю, что когда я помещаю свой код для проверки, я получаю где-то 100-200 комментариев. Верхнее число для меня было 300 комментариев один раз. Конечно, вы могли бы подумать, что это большие изменения, но это также могут быть очень маленькие изменения с менее чем 50 строками кода (включая модульные тесты). Все комментарии считаются «должны делать» и без аргументов.

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

Итак, мой вопрос, если это справедливо? Или обычный?

user1207047
источник
3
Какие комментарии вы получаете? Похоже, много. Это форматирование комментариев? Кодирование? Трудно ответить, не зная больше о природе комментариев (и, возможно, именно то, что в вашем коде вызвало комментарии).
MetalMikester
1
Привет - не уверен, что это правильный термин, но это в основном общие "лучшие практики", такие как переименование переменных, перемещение функций, переименование функций вверх в 3-5 раз и т. Д. У нас установлен phpcs, поэтому форматирование правильное.
user1207047
Также забыл упомянуть в этом билете, я на самом деле разработчик уровня 3 в этой компании. У меня есть сертификат php, и я отлично справляюсь за последние 8 лет работы здесь. Это только недавно стало происходить. Я имею в виду, что хотелось бы думать, что через 8 лет ты что-нибудь узнаешь правильно?
user1207047
1
«То есть я хотел бы думать, что через 8 лет ты что-нибудь узнаешь, верно?» - Ну, вы были бы удивлены ... Вещи, которые я иногда вижу на работе ...
MetalMikester

Ответы:

15

Все комментарии считаются «должны делать» и без аргументов.

ИМХО, это реальная проблема, поскольку в этом нет приоритетов. Когда вы получаете 100-300 комментариев, некоторые из них должны иметь приоритет A (реальные ошибки), некоторые из них - prio B (которые могут привести к ошибкам позже), а некоторые - prio C (все остальное). Скажите своим коллегам, что вы готовы уважать все их пожелания, но чтобы изменения были эффективными, а ваше время ограничено, настаивайте на расстановке приоритетов. Затем начните с исправления комментария prio A, и если после этого у вас действительно будет время на большее, вы можете начать с B (если вам повезет, ваш босс поймет, что исправление prio B и C не так важно, и даст вам некоторые более важные задачи вместо того, чтобы тратить свое время).

Док Браун
источник
Я много раз пытался попросить приоритет комментариев. Я получаю обратно что-то вроде «приятно иметь» и «требуется». Оказывается, подавляющее большинство из них "требуется".
user1207047
2
Я видел, как это происходит, когда конкретный разработчик получает множество элементов действий из своих обзоров, чтобы они не испортили код в других областях программы. Но это было бы для исключительно бедного разработчика, который «навязан» проекту, и руководство не может избавиться от них из-за управленческих решений.
Данк
2
Вы знаете @Dunk, я думаю, что вы здесь. Ваш комментарий действительно получил признание, и я принял этот ответ, так как не думаю, что могу принять комментарий. Я «посторонний» в этой группе и теперь понимаю, почему внутренний круг становится лучше и быстрее, а отзывы - нет. Я был «вынужден» в эту команду руководством, да, и мы «вынуждены» работать вместе. Так что это звучит очень разумно и логично объяснить, почему это жестче. Это или я действительно воняю при кодировании. Единственный способ понять это - пойти в другую группу / компанию и убедиться в этом сам.
user1207047
4
@ user1207047: Вы не должны принимать ответ, потому что вам нравится один из комментариев ниже, поскольку он идет вразрез со стандартами и целью сайта (я думаю, что я чувствую образец здесь). Для этого есть функция комментариев upvote.
webbiedave
10

Обзоры кода могут быть спорным процессом.

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

Если это первое, я бы порекомендовал работать над разрешением (новая работа или новые процессы проверки кода).

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

JoeG
источник
Привет, хорошие мысли. Из того, что я могу извлечь, некоторые из них действительно вдумчивый анализ, но большинство из них выглядят как придирки, такие как движущиеся функции или функции переименования. Проблема в том, что когда они объясняют свой мыслительный процесс, это действительно имеет смысл, но между собой они не делают то же самое и делают те же ошибки, что и я.
user1207047
Более того, обзор кода настолько глубок, что я забываю, что делал, и создаю больше ошибок, исправляющих приложение из-за чрезмерных сотен комментариев. Например, однажды мне сказали переписать большую часть кода. До этого код был правильным и функциональным. После проверки кода и почти 150 комментариев исходная функция и правильность исчезли, и было добавлено множество ошибок. Когда я понял это и исправил их, мне в основном сказали: «Да, наш процесс проверки кода делает вас отличным программистом, потому что теперь вы возвращаетесь к его исправлению, и это легче сделать».
user1207047
8
@user: важно называть имена методов / функций, это не обязательно придирки. Если вы плохо справляетесь с именами, это может раздражать вашу команду. Если вы не можете придумать четкое имя, то это, вероятно, не очень хорошая функция. Вы, кажется, «новый» парень, а у других, очевидно, есть метод их безумия, который они, вероятно, обсуждали много раз прежде. Таким образом, причина для меньшего количества комментариев. Я предлагаю вам узнать, что они хотят, и попытаться соответствовать, а не прикладом головы. Заработайте некоторое уважение, и тогда вы сможете предложить альтернативные идеи, которые встретят с открытым сердцем.
Данк
1
@user: Похоже, вам нужны стандарты кодирования / дизайна.
Данк
2
@user: Все, что вы можете сделать, это попытаться работать в системе и продемонстрировать, что вы командный игрок. Если ты это сделал. то либо ваше восприятие не является правильным, вы имеете дело с иррациональными людьми, они воспринимают ваше отношение к быть спорными или это просто гадкая службой политик. Единственное, что вы контролируете - это ваше отношение / восприятие. Если вы убеждены, что вы ни в коем случае не спровоцировали проблему, тогда я не знаю, почему вы остаетесь. Найдите место, в котором приятно работать, потому что люди ладят. Если такие же проблемы возникают в другом месте, посмотрите в зеркало.
Данк
5

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

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

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

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

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

Хуанми Родригес
источник
1
100% согласны с последним абзацем: вы должны обсудить намеченный вами проект до реализации. По крайней мере, тогда вы начинаете с предположительно приемлемой основы. Затем, после реализации, возможно, стоит обсудить окончательный дизайн (не код). Затем измените код, чтобы он соответствовал результатам финального обсуждения дизайна. Если после нескольких попыток это не помогло, это должно дать понять, что позиция просто плохая, и вы должны начать искать в другом месте.
Данк
4

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

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

  • Прежде всего, статический анализатор кода должен использоваться для выявления большинства проблем до того, как произойдет проверка кода. Например: запуск вашего кода через Sonar, Lint или любой другой хороший анализатор кода должен помочь вам избавиться от большинства мелких проблем. Тем более, что ваши рецензенты могут определять собственные профили, чтобы обеспечить все, от размещения скобок, пробелов, комментариев, правильного именования переменных и многого другого ...
  • Во-вторых, я, кажется, работаю хорошо, если вы разделите комментарии на разные категории. Например, две категории, где в одну группу входят мелочи, которые вы должны принять к сведению и применять в будущем. И вторая группа для тех комментариев, которые требуют немедленной модификации вашего кода, что потребует еще одного коммита и последующего просмотра. Конечно, количество комментариев в последней группе должно быть меньше.

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

¹ По моему опыту, это часто случается, так как многие программисты утверждают, что php - самый злой язык программирования, и его используют самые неопытные программисты. Я дистанцируюсь от этого утверждения, так как считаю, что отличное программное обеспечение может быть написано на любом языке!

² Предполагая, что, хотя комментарии являются чрезмерными, некоторая ценность в них

Жером
источник
Я искренне согласен с тем, что вы сказали. Это опыт обучения, и нужно учиться. Тем не менее, это продолжалось достаточно долго до такой степени, что кажется, что это не так. Либо я тупею, либо что-то еще происходит. Я предполагаю, что если каждый запрос на получение генерирует сотни комментариев, то либо вы все время ошибаетесь, либо здесь присутствует что-то еще, что не совпадает с тем, что, как они утверждают, они пытаются сделать. Либо они должны сказать: «Хорошо, давайте остановимся и учимся», либо перейдем к сути. По крайней мере, так я это вижу.
user1207047
1
@ user1207047 После прочтения ваших ответов на другие ответы, мне кажется, что вы уже знаете ответ на свой вопрос .. :-) Кажется, совершенно ясно, что что-то не так с вашими отзывами кода. Может быть, пришло время поговорить с начальником или попросить передачу в другую команду?
Жером
3

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

Однако это также зависит от «правил» процесса проверки кода. У ВСЕХ есть свои идеи о том, как что-то должно было быть сделано. Если ваш процесс проверки кода позволяет комментариям иметь форму «Вы должны делать это таким образом, а не таким образом», то вы, вероятно, получите МНОГО комментариев даже для адекватного кода. Если ваш процесс предназначен для поиска «дефектов», то количество комментариев должно быть намного меньше.

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

ОБНОВЛЕНИЕ: С учетом всего вышесказанного, некоторый код просто плохой, даже если без дефектов. В этом случае комментарий должен быть одним комментарием, который говорит что-то вроде. «Этот код необходимо очистить. Пожалуйста, отложите проверку до тех пор, пока код не будет обсужден с [вашим именем здесь]». В этом случае дальнейшее рассмотрение кода должно быть прекращено до исправления комментария.

ОБНОВЛЕНИЕ2: @User: Обсуждаете ли вы свой код / ​​дизайн с одним из них во время его разработки, чтобы вы могли реализовать то, что они ищут, прежде чем вы продолжите делать это по-своему? Вы что-то меняете в том, как вы разрабатываете код, основываясь на их предложениях, или продолжаете думать, что у вас все в порядке? Вы изучаете что-нибудь из их комментариев?

Когда я отвечаю за проект, моя работа - отвечать за ВСЕ рабочие продукты. Если я одобряю рабочий продукт, то я утверждаю, что продукт приемлем. Я хочу иметь репутацию производителя качественной продукции. Таким образом, у меня есть ожидания, и я не приму меньше, чем удовлетворительное. В то же время я стараюсь учить и объяснять причины своих предпочтений. Эти предпочтения не всегда могут быть идеальными (особенно в глазах других), но большинство этих предпочтений основано на опыте. Обычно это реакция, чтобы избежать повторения плохих. Таким образом, есть некоторые из моих личных "сторонников", которые необходимы, чтобы получить мое одобрение, независимо от откатов.

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

Замочить
источник
Согласились, и вы не услышите от меня никаких аргументов на том основании. Однако процесс не совсем такой. Они говорят, что это так, и в большинстве случаев оказывается, что только люди за пределами этих трех групп находятся под более пристальным вниманием, чем они сами. Они утверждают, что другие являются плохими разработчиками, но они являются единственными «разработчиками» в команде.
user1207047
Однако одна вещь состоит в том, что если вы не можете понять код, или разработчик заново изобрел колесо вместо использования существующего метода, или если его метод имеет цикломатическую сложность 50, то это, безусловно, имеет место для комментария, даже если нет ошибки Трудно читаемый код и дублирование - это ответственность, даже если это не ошибка. Вот почему я не стесняюсь указывать, что переменная имеет неправильное имя или что решение вводит временную связь, которая усложняет понимание кода. Технический долг должен быть управляемым.
Лоран Бурго-Рой
1
@ Лоран: Я знаю, что вы говорите, и во многом согласен. Однако, это открывает банку с червями, которые имеют тенденцию к снежному кому. Если у вашей компании есть средства и график, позволяющий проверке кода занять значительную часть усилий, это нормально (например, проекты в области медицинского оборудования / самолетов). Но большинство проектов не имеют роскоши. Таким образом, ограничение объема комментариев обзора очень полезно. Чтобы компенсировать ваши проблемы, это задача лидера по надзору за разработчиками и их работой. Они должны знать, кого следует отслеживать наиболее внимательно, и исправлять эти проблемы перед проверкой кода.
Данк
Мы должны согласиться не соглашаться здесь :). Технический долг - это то, что вы должны будете заплатить рано или поздно (и чем больше вы ждете, тем больше вы платите процентов). Вы не сэкономите ни копейки, затягивая уборку. Если вы не потратите время на его очистку, то следующее изменение может стоить вам вдвое больше времени, потому что вам будет сложно разобраться в коде. Я работаю с 8-летней базой кода, и из-за проблем с качеством разработка остановилась. Теперь у нас есть официальное правило «внутреннее качество не является неотъемлемым». Я могу засвидетельствовать, что это спасло нас!
Лоран Бурго-Рой
Я перечитал ваш комментарий и понял, что, возможно, у нас другой взгляд из-за нашей методологии. Я работаю в Agile Team, где нет лидерства. Поскольку мы все равны и все несем ответственность за качество кода, мы все должны контролировать друг друга. И проверка кода выполняется каждые 3-4 часа перед каждой интеграцией. Таким образом, очистка большого запроса на удаление - это несколько часов, если мы очень нацисты или мы сделали рефакторинг, который повлиял на старую и жесткую часть программного обеспечения. Следовательно, почему я вижу комментарий к качеству кода как «ничего страшного».
Лоран Бурго-Рой
2

Некоторые важные различия с процессом проверки нашей команды:

  • Основой проверки является контрольный список, составленный всей командой.
  • Фокус - дефекты (настоящее и будущее), а не стиль ради стиля.
  • 3 инспектора (включая автора) сидят вместе, чтобы просмотреть комментарии. Остались только комментарии с большинством голосов.
Крис Ван Баел
источник
2

Для 50 LOC 300 комментариев кажутся немного чрезмерными и - вау - 3 рецензента на каждый запрос на выборку? Ваша компания должна иметь много ресурсов.

Исходя из моего опыта для полезного процесса проверки кода, должны быть некоторые правила и / или рекомендации:

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

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

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

Если ваша команда разработчиков не согласилась с «проблемами вкуса» (например, если переменная-член начинается с «m_»), то каждый рецензент заставит вас следовать его стилю, который является пустой тратой времени / денег.

Саймон
источник
1

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

Я предлагаю сэкономить время. Ускорьте обратную связь. Я буду:

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

Люди могут возразить против спаривания, потому что «это займет больше времени», но это, очевидно, не проблема здесь.

Стив Джексон
источник