Как найти положительные моменты в обзоре кода?

183

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

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

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

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

Наш менеджер - MBA, который в основном занимается составлением графика предстоящих проектов. В то время как он пытается, он почти не имеет представления о том, что делает наше программное обеспечение с точки зрения бизнеса, и изо всех сил пытается понять даже самые основные требования клиентов без объяснения со стороны разработчика.

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

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

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

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

Я не думаю, что мои стандарты слишком высоки.

Мой контрольный список на данный момент:

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

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

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

Но мне трудно найти что-то хорошее. «Эй, на этот раз ты действительно совершил все, что сделал» - это скорее снисходительно, чем приятно или полезно.

Пример проверки кода

Эй Джо,

У меня есть несколько вопросов о ваших изменениях в классе Library \ ACME \ ExtractOrderMail.

Я не понял, почему вы отметили «TempFilesToDelete» как статический? В настоящий момент второй вызов GetMails вызовет исключение, потому что вы добавляете файлы в него, но никогда не удаляете их после того, как удалили их. Я знаю, что функция вызывается только один раз за запуск, но в будущем это может измениться. Можете ли вы просто сделать его переменной экземпляра, тогда у нас может быть несколько объектов параллельно.

... (некоторые другие пункты, которые не работают)

Незначительные баллы:

  • Почему «GetErrorMailBody» принимает исключение в качестве параметра? Я что-то пропустил? Вы не генерируете исключение, вы просто передаете его и вызываете ToString. Почему это?
  • SaveAndSend Не подходит для метода. Этот метод отправляет сообщения об ошибках, если обработка почты прошла неправильно. Не могли бы вы переименовать его в «SendErrorMail» или что-то подобное?
  • Пожалуйста, не просто комментируйте старый код, просто удалите его. У нас все еще есть это в подрывной деятельности.
RobMMurdock
источник
8
Пожалуйста, не подавайте сэндвич за $ h! T, чтобы достичь мифического баланса добра и зла. Если они сделали что-то хорошее, скажите им об этом, если они сделали что-то, что требует исправления, дайте им знать. Смешивание хорошего и плохого разбавляет сообщение. Если они получат гораздо больше отрицательных отзывов, чем положительных, возможно, они поймут, что им нужно измениться. Ваш подход сэндвича дает соотношение 2: 1 для каждого негатива, так что в итоге получается, что сообщение, которое вы хотите отправить.
cdkMoose
14
Прекратить использование 2-го лица. Код является темой, а не кодером. Например, напишите: SaveAndSend должен быть переименован, чтобы лучше соответствовать его поведению, как, например, SendErrorMail . Прямо сейчас действительно похоже, что вы отдаете приказы своему коллеге, даже несмотря на то, что вы «могли бы, пожалуйста», что вы пролили повсюду. Я бы не принял это от рецензента. Я предпочитаю кого-то, кто прямо заявляет: «Это должно быть сделано», а не просит меня (даже вежливо) что-то сделать.
Артур Гавличек
4
«Я читал, что нужно стараться смешивать плохие новости с хорошими новостями». Вам нужно убедиться, что есть ясное, глобальное понимание того, что это не то, чем являются обзоры кода. Они не похожи на обзоры эффективности работы сотрудников или обзоры фильмов, которые весят хорошо и плохо. Они больше похожи на процесс обеспечения качества. Вы не ожидаете, что ваши тестеры создадут тикеты с надписью «Эта функция великолепна и работает так, как я ожидаю!», И вы не должны ожидать этого и в обзорах кода.
Бен Ааронсон
3
Я думаю, что ваш первый шаг должен состоять в создании базового набора стандартов / руководств по кодированию, чтобы другие участники могли предоставить обратную связь, и в основном получить «согласие» / согласие от всех, что руководящие принципы «в пределах разумного». Затем они все знают, что они согласились остаться с ними. Это хорошо сработало в пред. компания, в которой я работал.
code_dredd
3
Не используйте эту фразу «но в будущем это может измениться». Код только для того, что нужно сейчас. Не создавайте сложности для будущих изменений, которые могут произойти или не произойти. Если вы точно знаете, что это изменится, то это другое, но не случайно, что это может измениться.
Дом Декстера

Ответы:

123

Как найти положительные моменты в обзоре кода?

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

Отлично, у вас есть реальная возможность создать ценность для вашей фирмы.

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

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

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

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

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

Критикуй код, а не автора

Вы приводите пример:

У меня есть несколько вопросов о ваших изменениях в

Избегайте использования слов «вы» и «ваш», скажем, «изменения» вместо.

Я что-то пропустил? [...] Почему это?

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

Может быть, вы защищаете свое эго за чужой счет. Придерживайтесь только фактов.

Поднимите планку, дав положительный отзыв

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

Как найти положительные моменты в обзоре кода?

хороший, и стоит обратиться.

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

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

Лучшие языковые практики

Если язык поддерживает документацию в коде, пространствах имен, объектно-ориентированных или функциональных функциях программирования, вы можете вызвать их и поблагодарить автора за их использование в случае необходимости. Эти вопросы обычно подпадают под руководства по стилю:

  • Соответствует ли оно внутренним стандартам языкового руководства по стилю?
  • Соответствует ли оно наиболее авторитетному руководству по стилю для языка (которое, вероятно, является более строгим, чем внутренний - и, следовательно, по-прежнему соответствует внутреннему стилю)?

Общие лучшие практики

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

Искать:

  • модульные тесты, которые тестируют только предметную функциональность - насмешливая дорогая функциональность, которая не предназначена для тестирования.
  • высокий уровень покрытия кода, с полным тестированием API и семантически публичной функциональностью.
  • приемочные испытания и дымовые испытания, которые тестируют сквозную функциональность, включая функциональность, которая проверяется для модульных испытаний.
  • хорошее наименование, канонические точки данных, поэтому код СУХОЙ (не повторяйте себя), никаких волшебных строк или чисел.
  • именование переменных настолько хорошо сделано, что комментарии в значительной степени излишни.
  • исправления, объективные улучшения (без компромиссов) и соответствующие рефакторинги, которые сокращают количество строк кода и технические долги, не делая код полностью чуждым оригинальным авторам.

Функциональное программирование

Если язык функционален или поддерживает функциональную парадигму, ищите эти идеалы:

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

Объектно-ориентированное программирование (ООП)

Если язык поддерживает ООП, вы можете похвалить соответствующее использование этих функций:

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

под ООП также существуют ТВЕРДЫЕ принципы (возможно, некоторая избыточность к функциям ООП):

  • единственная ответственность - у каждого объекта есть один участник / владелец
  • открыть / закрыть - не изменять интерфейс установленных объектов
  • Лисковская подстановка - подклассы можно заменить на экземпляры родителей
  • разделение интерфейса - интерфейсы, предоставляемые композицией, возможно, mixins
  • инверсия зависимостей - определены интерфейсы - полиморфизм ...

Принципы программирования Unix :

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

В целом, эти принципы могут применяться во многих парадигмах.

Ваши критерии

Это слишком тривиально - я бы чувствовал снисходительность, если бы похвалили за это:

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

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

  • Код будет работать с большинством крайних случаев.
  • Код будет выдавать разумное исключение, если вставленные данные недействительны.

Записываете правила прохождения проверки кода?

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

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

Заключение

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

Аарон Холл
источник
8
Я бы даже сказал, что многие из них могут быть заголовками в шаблоне отзыва о проверке кода. Это дает возможность для комментариев, таких как «отличная работа» под несколькими заголовками, без каких-либо реальных дополнительных затрат. Это также дает коллегам хорошее представление о том, как улучшить их код.
Стивен
9
Перечисляя множество хороших практик, вы, вероятно, отвечаете не на тот вопрос - потому что это действительно проблема xy. И трудно найти систему обзора, которая позволила бы эти отзывы. Важные вещи скрыты в бесполезном шуме. Иногда ответом на вопрос является просто «Не делай этого - это неправильный путь. Твоя проблема лежит в другом месте и должна быть адекватно решена». Если люди начинают концентрироваться на поиске хороших вещей, проверка кода становится пустой тратой времени. Вы можете сказать своему коллеге во время обеда, насколько хороша его реализация, и он может оценить это.
Эйко
4
@ Аарон: согласен с вами в подходе. Многие ответы здесь говорят «не приукрашивайте это», но я понимаю, что это не для того, чтобы угодить всем. Люди более склонны следовать правильному подходу, когда добрые дела подкрепляются ими, а не когда им говорят, что они неправы. Ключевым моментом здесь является тактичность, но последовательность в отношении того, что делать. Судя по описанию ОП, он находится в менее чем совершенной команде программистов, и даже старые участники привыкли к этому. Они были бы более восприимчивы к нежному подходу.
Hoàng Long
@ HoàngLong Не каждый «старый таймер» обязательно будет «более восприимчивым». Там всегда кто-то неразумный где-то. Например, я работал с парнем, который настаивал на том, чтобы «переносить» свои лучшие практики Perl на Python, а Subversion на Git, и каждый раз получал жалобы, независимо от того, как это было, даже если рассуждения были объяснены. Поскольку в то время ответственность за это легла на мои колени (я был единственным, кто имел опыт работы с Python и Git), я думаю, что некоторые люди могут просто чувствовать угрозу (?) И реагировать соответствующим образом ...
code_dredd
104

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

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

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

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

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

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

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

plast1k
источник
4
Другая стратегия, которую я нашел полезной как рецензент и как рецензируемый, заключается в том, чтобы объяснить необходимость покрытия крайних случаев из-за третьей стороны. Я приношу извинения тем, кто занимает руководящие должности, но говорю такие вещи, как «мы должны учитывать этот крайний случай, потому что руководство действительно преследует наши цели, поэтому мы хотим убедиться, что это почти пуленепробиваемое. Дает им ощущение легкости». Также звучит так, как будто руководство не возражало бы быть "плохим парнем" в случае ОП в любом случае.
Грег Бургхардт
7
@GregBurghardt Эй, они не зря называют это офисной политикой.
plast1k
30
Я согласен с тем, что вы здесь говорите, и это становится еще дальше, но я думаю, что важно помнить, что обзоры кода не должны быть состязательными. Это два человека, которые сидят с общей целью - создать хороший код и хороший продукт. Вы можете иногда не соглашаться с тем, является ли тот или иной подход лучше, но все аргументы обеих сторон должны основываться на правильных действиях для команды, компании и / или клиента. Если вы оба можете согласиться с этим, это более плавный процесс.
Хоббс
6
«Не беспокойтесь о том, чтобы выбрать что-то хорошее, если только это не будет четким кратким примером и напрямую связано с целенаправленной проблемой». - Я думаю, что открытие немного резкое. Когда я делаю обзор кода, я всегда «стараюсь» начинать с чего-то положительного, даже мне приходится прибегать к чему-то мягкому. Это помогает установить тон и показывает, что вы не просто ищете негативные аспекты кода.
Брайан Оукли
2
«Убедитесь, что вы обвиняете код, а не автора». Согласен, но неуверенный / незрелый вид не пойдет на это.
MetalMikester
95

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

Единственный способ избежать этого - записать правила прохождения проверки кода.

Тогда это не тот человек, который терпит неудачу или одобряет регистрацию. Они просто проверяют соблюдение правил.

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

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

Ewan
источник
56
«Единственный способ избежать этого - записать правила прохождения проверки кода». Этот. Вы должны пересматривать все в соответствии с некоторыми стандартами, которые были установлены для проекта в целом, а не против ваших личных представлений о том, что хорошо, какими бы глубокими не были ваши личные идеи.
alephzero
6
Вопрос в том, как найти положительные вещи. Откуда ты знаешь, что имя достаточно хорошее? Когда имя слишком плохое, чтобы пройти проверку кода? Многие вещи, которые он мог бы похвалить, слишком субъективны, чтобы иметь жесткое и быстрое правило. Таким образом, я не думаю, что это отвечает на вопрос.
Аарон Холл
20
-1 Мне нравится, как вы прыгаете от критики «войн ботаников», а потом говорите: «Единственный способ избежать этого».
тымтам
33
Невозможно записать правило для всех возможных неудачных дизайнерских решений. И если вы попытаетесь сделать его по ходу дела, вы быстро обнаружите, что документ становится непригодным для использования из-за большой длины. -1
jpmc26
15
Гораздо более полезными, чем стандарты кодирования, являются разработчики и рецензенты, которые могут действовать как настоящие взрослые.
gnasher729
25

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

На мой взгляд, лучшая практика - всегда сосредотачиваться на коде, а не на авторе.

Это обзор кода , а не обзор разработчика , поэтому:

  • «Этот цикл может никогда не закончиться», а не «Ваш цикл может никогда не закончиться»
  • «Тестовый случай необходим для сценария X», а не «Вы не написали тест, чтобы охватить этот сценарий»

Также очень важно применять одно и то же правило, обсуждая обзор с другими:

  • «Энн, что ты думаешь об этом коде?», А не «Энн, что ты думаешь о коде Джона?»

Проверка кода - не время для проверки производительности - это следует делать отдельно.

tymtam
источник
3
Вы на самом деле не отвечаете на вопрос. Вопрос «Как найти положительные моменты в обзоре кода?» - и этот ответ просто противоречие - вы отвечаете: «Как я могу дать отрицательный отзыв».
Аарон Холл
15

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

Почему вы просматриваете весь запрос на слияние в одном сообщении?

Мой опыт с обзорами кода через GitLab; Я всегда думал, что другие инструменты для проверки кода будут работать аналогично.

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

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

Благодаря этому рабочему процессу проверки кода становятся гораздо более модульными и менее связными. Строка из обзора кода может просто сказать:

Хороший подход, завернув это в специальную функцию!

Или это может сказать,

Это имя объекта на самом деле не соответствует назначению объекта; может быть, вместо этого мы могли бы использовать имя типа 'XYZ'?

Или, если есть серьезные проблемы с разделом, я мог бы написать:

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

(Пример: функция ABC на самом деле делает три вещи: базирование foo, запрет боз и копирование zorf. Все это могут быть отдельными функциями.)

После написания всего вышесказанного я могу сделать краткий комментарий ко всему запросу на слияние, что-то вроде:

Это отличная новая функция, которая будет действительно полезна после объединения. Можете ли вы очистить именование функций и обработать рефакторинг, упомянутый в отдельных комментариях, которые я сделал, а затем дайте мне знать, чтобы посмотреть на него снова? :)


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

Извините, но этот код не совсем подходит. Существует очень много крайних случаев (как подробно описано в отдельных комментариях), которые будут обрабатываться неправильно и приводить к плохому пользовательскому опыту или даже повреждению данных в одном случае. (См. Комментарий к коммиту 438a95fb734.) Даже некоторые обычные сценарии использования приведут к крайне низкой производительности приложения (подробности отмечены в отдельных комментариях в diff для somefile.c).

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

Я закрываю запрос на слияние до полной перезаписи.


Резюме: Просмотрите технические аспекты кода в виде комментариев к отдельным строкам кода. Затем суммируйте эти комментарии в общем комментарии к запросу на слияние. Не переходите на личности - просто разберитесь с фактами и, по вашему мнению, о коде , а не о кодере. И основывайте свое мнение на фактах, точных наблюдениях и понимании.

Wildcard
источник
12

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

Просто нет причин, по которым вам следует обращаться к Джо. То, что Джо исправляет свои ошибки, не твое дело. То, что кто-то делает, это ваше дело. Ваша забота о качестве кода. Таким образом, вместо того, чтобы писать запросы / приказы / требования (что, если бы я был Джо, я мог бы просто отказаться, потому что вы не имеете права на это), оставайтесь на своей должности и напишите простой анонимный список задач.

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

Вот пример переформулировки с содержанием взятым из вашего обзора:

  • Прежде чем мы внесем изменения в класс Library \ ACME \ ExtractOrderMail, нам нужно исправить несколько проблем.
  • Если я что-то пропустил, «TempFilesToDelete» не должен быть статичным.
  • В будущем мы можем вызывать функцию более одного раза за запуск, поэтому нам это нужно (что нужно сделать здесь).
  • Мне нужно понять, почему «GetErrorMailBody» принимает исключение в качестве параметра. (и я здесь на грани, потому что к настоящему времени у вас уже должно быть заключение )
  • SaveAndSend должен быть переименован, чтобы лучше соответствовать его поведению, как, например, «SendErrorMail»
  • Закомментированный код должен быть удален для удобства чтения. Мы используем Subversion для возможных откатов.

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

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

Артур Гавличек
источник
Обзор образец недавнее дополнение к этому вопросу, большинство не отвечающими видел
Izkata
8

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

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

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

Это действительно культурная вещь, и она требует большого доверия и общения. И время поработать с результатами.

Эйко
источник
2
«Весь смысл проверки кода состоит в том, чтобы найти проблемы», правда, - но ни один из них не отвечает на вопрос в том виде, как он был задан.
Аарон Холл
3
Он спрашивает неправильный ху-проблемы см meta.stackexchange.com/questions/66377/what-is-the-xy-problem
Эйко
1
Ваша команда (менеджер) должна сообщить, что создание ошибок - это часть игры, но их поиск и исправление спасут работу каждого . Это правда, и это означает, что каждый является заинтересованным лицом. Но ответственность за то, чтобы кто-то указал на ошибку (или просто плохой спагетти-код), не написана, чтобы доказать оригинальному кодеру, что это ошибка. (только если он широко оспаривается , что он действительно является ошибкой.)
Роберт Бристоу-Джонсон
6

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

Что-то вроде соглашений об именах, спросите других, важно ли это. Большинство разработчиков согласятся, особенно среди своих коллег. Кто хочет быть человеком, который не хочет соглашаться с чем-то настолько распространенным в мире программирования? Когда вы начинаете процесс, выбирая чей-то код и указывая на недостаток, они становятся очень оборонительными. Когда стандарты будут установлены, возникнут разногласия по поводу интерпретации или того, что считается «достаточно хорошим», но вы в лучшем положении, чем сейчас.

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

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

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

JeffO
источник
4

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

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

Еще одна вещь; может быть защитный воздух, потому что они чувствуют, что у них нет права голоса. Почему бы не позволить им просмотреть код друг друга? Задайте им конкретные вопросы и постарайтесь вовлечь их. Это не должен быть ты против них; это должно быть командным усилием.

  1. Что бы вы изменили в этом коде, если бы у вас было время?
  2. Как бы вы улучшили эту область кодовой базы?

Спросите это сейчас, и спросите это через шесть месяцев. Здесь есть опыт обучения.

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

lunchmeat317
источник
1
«не беспокойтесь о том, чтобы дать хорошую обратную связь, если нет ничего хорошего для измерения», мне трудно поверить, что он не смог найти хотя бы чего-то положительного, чтобы сказать о коде, написанном другими профессиональными программистами, в то же время поднимая планку ожиданий для всех. код. Это не отвечает на вопрос - это просто противоречие.
Аарон Холл
2
@AaronHall: «Ваш код может служить хорошим примером того, как не писать код». Это достаточно позитивно?
gnasher729
1
@AaronHall Если ОП может найти что-то положительное в отношении кода, написанного другими профессиональными программистами, то он должен это сделать. Однако, если нет, то нет смысла пытаться что-то придумать. Вместо этого OP должен сосредоточиться на усилиях и обучении разработчиков, а не на самом коде.
lunchmeat317
4

Качество без напряжения

Вы спросили, как вы можете найти положительные отзывы о коде, но ваш реальный вопрос заключается в том, как избежать «напряженности в [вашей] команде» и одновременно решить «серьезные проблемы с качеством».

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

Проблемы вашей организации сверху вниз

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

Зачем спрашивать нас, что вам нужно сделать, чтобы ваша команда была счастлива? Рассматривали ли вы вопрос своей команды?

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

Создание культуры качества

Культура качества должна развиваться, чтобы расти снизу вверх.

Сообщите своему боссу, что вы обеспокоены тем, что качество не улучшается достаточно быстро, и вы хотите попробовать воспользоваться рекомендацией The Harvard Business Review .

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

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

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

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

Совместные обзоры кода

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

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

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

Качество - это путешествие

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

Но если это не сработает ...

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

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

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

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

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

Точно так же «я бы сделал это по-другому ...» также конфронтационно, потому что у разработчика сразу возникнет мысль: « Ну, я сделал это так ... У вас проблемы с этим? » И опять же, это более конфронтационно, чем это должно быть и отвлекает обсуждение от улучшения кода.

Пример:

Вместо того, чтобы спрашивать «Почему вы решили не использовать постоянную переменную для этого значения?», Просто заявите: «Это жестко запрограммированное значение должно быть заменено константой XYZв файле Constants.h». Задавая вопрос, можно предположить, что разработчик активно решил не использовать уже определенная константа, но вполне возможно, что они даже не знали, что она существует. С достаточно большой базой кода каждый разработчик может многое не знать. Это просто хорошая возможность обучения для этого разработчика, чтобы познакомиться с константами проекта.

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

Хороший:

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

  • «В этом названии функции буква« b »должна быть написана заглавными буквами, чтобы быть буквой PascalCase».

  • «Код этой функции не имеет правильного отступа».

  • «Этот код является дубликатом кода, найденного в ABC::XYZ(), и должен использовать эту функцию вместо этого».

  • «Вы должны использовать try-with-resources, чтобы в этой функции гарантированно закрывались все вещи, даже если возникают ошибки». [Я только добавил сюда ссылку, чтобы пользователи, не являющиеся Java, знали, что означает попытка с ресурсами]

  • «Эта функция должна быть реорганизована, чтобы соответствовать нашим стандартам сложности n-path».

Плохо:

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

  • « Возможно, было бы лучше использовать try-with-resource для правильного закрытия объектов в этой функции»

  • «Возможно, было бы желательно еще раз взглянуть на все условия в этой функции. Его сложность N-пути превышает максимально допустимую сложность нашего стандарта».

  • «Почему отступы здесь 2 пробела вместо 4 нашего стандарта?»

  • «Почему вы написали функцию, которая нарушает наш стандарт сложности n-path?»

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

«Хорошие» высказывания являются грубыми, но они не обвиняют разработчика, они не нападают на разработчика, они не конфронтационны и не задаются вопросом, почему существует дефект. Это существует; вот исправление. Они, конечно, не такие конфронтационные, как последний вопрос «почему».

Shaz
источник
1
Многие из приведенных вами примеров будут обнаружены статическим анализом. По моему опыту, вещи, которые возникают в обзорах кода, часто бывают более субъективными и самоуверенными: «Я бы назвал XY вместо этого, потому что я думаю, что это лучше отражает поведение». В нашей организации создатель PR может использовать свое собственное суждение и либо изменить имя, либо оставить его как есть.
Мутон
@ Мутон Я согласен с вами по поводу статического анализа. У нас эти проверки автоматизированы в проектах, над которыми я работаю. У нас также есть инструменты, которые автоматизируют форматирование кода, поэтому большинство проблем со стилем кодирования никогда не возникает (или редко). Но OP особо упомянул, что их кодовая база является беспорядком, поэтому я предположил, что с такими проблемами они сталкиваются в обзорах, и я думаю, что эти простые примеры остаются актуальными для обновления OP, сделанного для демонстрации примера обзора.
Shaz
3

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

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

«Почему метод должен иметь разумное имя, я знаю, что он делает?» это то, что я нахожу особенно тревожным. Он знает, что он делает, или, по крайней мере, он так говорит, но я не знаю. Любой метод должен иметь не только разумное имя, но и имя, которое сразу же дает читателю кода понять, что он делает. Возможно, вы захотите зайти на сайт Apple и посмотреть видеоролик WWDC об изменениях с Swift 2 на Swift 3 - огромное количество изменений, внесенных просто для того, чтобы сделать все более читабельным. Возможно, такого рода видео могло бы убедить ваших разработчиков в том, что разработчики, которые намного умнее их, считают интуитивно понятные имена методов очень и очень важными.

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

gnasher729
источник
+1 Разработчик может знать, что делает его субоптимально названная функция, но что происходит, когда он едет под автобусом?
Mawg
3

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

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

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

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

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

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

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

Дэвид В.
источник
+1 за рецензирование кода лично, а не в электронном виде - это поможет избежать критики
alexanderbird
3

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

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

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

Карл Билефельдт
источник
1

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

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

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

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

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

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

navigator_
источник
1

Предпосылки...

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

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

Генерируемые компьютером сообщения об ошибках редко подвергаются сомнению и никогда не воспринимаются как личное оскорбление.

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

Выводы ...

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

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

Пример кода Отзыв Отзыв ...

Переписать приведенный пример, включив эти методы:

  • Тема:

    • Библиотека \ ACME \ ExtractOrderMail Класс.
  • Принцип вопроса ...

    • TempFilesToDelete является статическим
      • Последующие вызовы GetMails выдают исключение, поскольку файлы добавляются в него, но никогда не удаляются после удаления. Несмотря на то, что только один вызов сейчас, производительность может быть улучшена в будущем с некоторым параллелизмом.
      • TempFilesToDelete в качестве переменной экземпляра позволит параллельно использовать несколько объектов.
  • Вторичные проблемы ...
    • GetErrorMailBody имеет параметр исключения
      • Поскольку он не генерирует исключение сам по себе, а просто передает его ToString, необходимо ли это?
    • SaveAndSend name
      • Электронная почта может использоваться или не использоваться для сообщения об этом в будущем, и этот код содержит общую логику для хранения постоянной копии и сообщения о любых ошибках. Более общее имя позволило бы такие ожидаемые изменения без изменения в зависимых методах. Одной из возможностей является StoreAndReport.
    • Закомментирование старого кода
      • Оставить строку с комментариями и пометить OBSOLETE может быть очень полезно при отладке, но «стена комментариев» также может скрывать ошибки в смежном коде. У нас все еще есть это в Subversion. Возможно, просто комментарий, указывающий, где именно в Subversion?
DocSalvager
источник
0

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

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

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

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

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

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

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

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

Бесполезный
источник
Независимо от того, используются ли отдельные исправления или нет, политика поврежденного окна должна быть унаследована из устаревшего кода, независимо от того, является ли эта политика «не исправлять разбитые окна» или «только в [методах / классах / файлах?], Которые затрагиваются текущим исправлением». ». По моему опыту, предотвращение исправления неработающими окнами разработчиков вредно для морального духа команды.
Деви Морган
1
Да, но принуждать их исправлять каждое разбитое окно в модуле до того, как их однострочный переход проходит проверку, одинаково токсично.
бесполезно
Согласовано! Я чувствую, что политика, которая была выбита командой, а не навязана извне, - единственный вид, который может работать.
Деви Морган
0

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

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

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

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

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

[1] Это не решается только потому, что люди сказали «хорошо» или перестали бороться за это. Никто не хочет быть парнем, чтобы сказать, что X просто не практичен для нашего {интеллекта, опыта, человеческой силы, сроков и т. Д.}, Но это не значит, когда речь идет о каком-то конкретном случае выполнения X ...

drjpizzle
источник
-1

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

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

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

Стиг Хеммер
источник
Не уверен, почему это было отклонено (отрицательные отзывы без комментариев глупы в теме о хорошем обзоре кода). Все рецензии должны быть стандартной процедурой. На доске Канбан будет колонка для проверки кода, и тот, кто в команде поднимает следующий элемент, должен сделать обзор (с оговорками; новички не должны собирать обзоры на некоторое время, а затем должны начинать с тех, которые требуют немного знаний предметной области). На доске схватки, по сути, похоже: работа справа налево.
Деви Морган
2
@DewiMorgan Я не согласен с тем, что "новички не должны собирать отзывы некоторое время". Новички, делающие обзоры, являются для них отличным способом познакомиться с базой кода. Тем не менее, они не должны быть единственным рецензентом! И, тем не менее, я в любом случае также опасаюсь, что в большинстве случаев у меня будет только один рецензент.
Разочарован