Согласование правил бойскаутов и оппортунистического рефакторинга с обзорами кода

55

Я большой сторонник правила бойскаутов :

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

Я также большой сторонник связанной идеи Оппортунистического Рефакторинга :

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

Особо обратите внимание на следующую выдержку из статьи о рефакторинге:

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

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

Я утверждаю, что преимущества того стоят, и что 3 рецензента будут работать немного усерднее (чтобы на самом деле понять код до и после, а не смотреть на узкую область видоизмененности строк - сам обзор был бы лучше благодаря одному этому ) так что следующие 100 разработчиков, читающих и поддерживающих код, получат пользу. Когда я представляю этот аргумент моим рецензентам, они говорят, что у них нет проблем с моим рефакторингом, если он не в том же CR. Однако я утверждаю, что это миф:

(1) В большинстве случаев вы понимаете, что и как вы хотите провести рефакторинг, когда находитесь в середине своего задания. Как говорит Мартин Фаулер:

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

(2) Никто не будет благосклонно смотреть на то, как вы выпускаете «рефакторинг» КР, которые вы не должны были делать. У CR есть определенные накладные расходы, и ваш менеджер не хочет, чтобы вы «тратили свое время» на рефакторинг. Когда это связано с изменением, которое вы должны сделать, эта проблема сводится к минимуму.

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

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

Любые мысли о том, как я могу исправить эту ситуацию?

t0x1n
источник
40
Я чувствовал бы себя неловко, работая в месте, гдеyour manager doesn't want you to "waste your time" on refactoring
Daenyth
19
В дополнение к наличию нескольких CR, ключевым моментом является то, что каждый коммит должен быть предназначен для одной цели: один для рефакторинга, один для требования / bug / etc. Таким образом, обзор может различать рефакторинг и запрашиваемое изменение кода. Я также утверждаю, что рефакторинг следует проводить только в том случае, если существуют юнит-тесты, которые доказывают, что ваш рефактор ничего не сломал (дядя Боб соглашается).
2
@ t0x1n Я не вижу в этом ничего особенного
Daenyth
2
@ t0x1n Да, я пропустил это. Не хватает кофе этим утром. По моему опыту есть несколько способов рефакторинга. Может быть, вы посмотрите на код, который нужно изменить, и сразу узнаете, что он нуждается в очистке, поэтому сначала сделайте это. Возможно, вам нужно что-то реорганизовать, чтобы внести изменения, потому что новое требование несовместимо с существующим кодом. Я бы сказал, что рефакторинг является неотъемлемой частью вашего изменения и не должен рассматриваться как отдельный. Наконец, возможно, вы видите, что код отстой в середине вашего изменения, но вы можете закончить его. Рефакторинг по факту.
6
Bullet 1 не утверждает, что разделение коммитов невозможно. Это просто означает, что вы не знаете, как это сделать, или ваша VCS делает это трудно. Я делаю это все время, даже принимая один коммит и разделяя его по факту.
бесполезно

Ответы:

18

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

ТЛ; др

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

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

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


Проблемы рабочего процесса / офисной политики

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

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


Проблемы с VCS

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

  1. если вы используете git, у вас есть отличные инструменты для этого

    • Вы можете использовать git add -iдля интерактивной постановки из командной строки
    • Вы можете использовать git guiи выбирать отдельные фрагменты и строки для стадии (это один из немногих инструментов графического интерфейса, связанных с VCS, который я предпочитаю командной строке, а другой - хороший редактор трехстороннего слияния)
    • Вы можете сделать множество мелких коммитов (отдельные изменения или исправить один и тот же класс ошибок в нескольких местах), а затем изменить их порядок, объединить или разделить их с помощью rebase -i
  2. если вы не используете git, ваша VCS может иметь инструменты для такого рода вещей, но я не могу посоветовать, не зная, какую систему вы используете

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

  3. Вы можете сделать это вручную в любой VCS, если у вас есть доступ к основным инструментам, таким как diffи patch. Это более болезненно, но, безусловно, возможно. Основной рабочий процесс будет:

    1. внесите все изменения, протестируйте и т. д.
    2. использовать diffдля создания (контекстного или унифицированного) файла патча со всеми изменениями с момента последнего коммита
    3. разделите это на два файла: в итоге вы получите один файл патча, содержащий рефакторинги, и один с функциональными изменениями
      • это не совсем тривиально - инструменты, такие как режим Emacs Diff, могут помочь
    4. поддержать все
    5. вернуться к последнему коммиту, использовать patchдля воспроизведения одного из файлов патчей, зафиксировать эти изменения
      • NB. если патч не был применен корректно, возможно, вам придется исправить неисправные блоки вручную
    6. повторите 5 для второго файла патча

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

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

Бесполезный
источник
Я не уверен, что следую - пуля 3.3 предполагает, что рефакторинг и функциональные изменения находятся в разных файлах? Во всяком случае, они этого не делают. Возможно, разделение по линиям имеет больше смысла, но я не думаю, что у нас есть инструменты для этого в нашем CVS (TFS). В любом случае, это не сработает для многих (большинства?) Рефакторингов, где функциональные изменения зависят от измененных рефакторингов. Например, предположим, что я рефакторил метод Foo (который мне нужно использовать как часть измененного функционала), чтобы взять 3 параметра вместо 2. Теперь функциональный код Imy опирается на код рефакторинга, даже разделение на строки не поможет.
t0x1n
1
Разные строки в одном и том же файле прекрасно подходят для заданных рабочих процессов. И учитывая, что две фиксации будут последовательными , вполне нормально, чтобы вторая (функциональная) фиксация зависела от первой. Да, и TFS2013 якобы поддерживает git.
бесполезно
Мы также используем TFS для контроля версий. Вы предполагаете, что второй коммит будет функциональным, тогда как обычно он будет противоположным (учитывая, что я не могу заранее сказать, какой рефакторинг должен быть сделан). Я полагаю, я мог бы выполнить всю свою работу по функционалу + рефакторинг, затем избавиться от чего-либо функционального и добавить его обратно в отдельный коммит. Я просто говорю, это много хлопот (и времени), просто чтобы пара рецензентов была счастлива. Более разумный подход, на мой взгляд, заключается в том, чтобы разрешить оппортунистический рефакторинг и взамен согласиться с CR на такие изменения самостоятельно (принимая на себя дополнительные трудности).
t0x1n
3
Я думаю, что ты действительно не понимаешь меня. Редактирование исходного кода и группирование изменений в коммиты, да и редактирование в одном и том же файле, являются логически отдельными действиями. Если это кажется трудным, вам просто нужно лучше изучить доступные инструменты управления исходным кодом.
бесполезно
1
Да, ваше понимание верно, у вас будет два последовательных коммита со вторым (функциональным) в зависимости от первого (рефакторинг). Описанный выше рабочий процесс diff / patch - это именно тот способ, который не требует ручного удаления изменений и последующей их перезаписи.
бесполезно
29

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

Любые мысли о том, как я могу исправить эту ситуацию?

Продолжать делать то, что ты делаешь?

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

Что может помочь, так это быть менее воинственным. Обзоры кода не должны быть воинственными «Зачем ты это сделал?», Они должны быть совместными «Эй, ребята, пока я был здесь, я все исправил!». Работать (с вашим лидером / менеджером, если возможно) над изменением этой культуры сложно, но очень важно создать высокофункциональную команду разработчиков.

Вы также можете работать (с вашим руководителем / менеджером, если это возможно), чтобы продвигать важность этих идей с вашими коллегами. Сделайте так, чтобы вы спросили: «Почему вы, ребята, не заботитесь о качестве?» вместо того, чтобы спросить: «Почему вы всегда делаете эти бесполезные вещи?».

Telastyn
источник
5
Да, ЧР большие и формальные. Изменения оформляются, подписываются, затем отправляются в очередь. Небольшие изменения должны добавляться как итерации к текущему CR, а не фиксироваться отдельно. Продолжая то, что я делаю, это действительно может принести пользу группе, но, боюсь, это не принесет мне пользы . Те люди, которым я "причиняю неудобства", вероятно, те же люди, которые оценили бы меня в ежегодном обзоре. Проблема с изменением культуры заключается в том, что в это верят крупные вожди. Возможно, мне просто нужно заслужить больше уважения в их глазах, прежде чем пытаться что-то подобное ...
t0x1n
13
@ t0x1n - Не смотри на меня. Я сделал карьеру делать правильные вещи в лице людей , которые упорно обязанных сосание. Может быть, не так выгодно, как могло бы, если бы я делал людей счастливыми, но я сплю хорошо.
Теластин
Спасибо за честность. Это действительно что-то для размышления.
t0x1n
1
Я часто сталкиваюсь с этим тоже. Обеспечение наличия патча «очистки», а затем рабочего патча очень помогает. Обычно я бьюсь внутри системы, а потом ухожу на работу куда-нибудь менее напряженную. Тем не менее, иногда есть веские причины для беспокойства ваших коллег. Например, если код быстро запускается в производство и нет достаточных тестов. Я рассматривал обзор кода как попытку избежать тестирования. Это не работает. Проверка кода помогает сохранить единообразие кода. Это мало для ошибок.
Шон Перри
1
@SeanPerry согласен - но я говорю о нормальных обстоятельствах, когда существуют тесты, выполняются баши ошибок и т. Д.
t0x1n
14

У меня есть много сочувствия к вашей ситуации, но мало конкретных предложений. Если ничего другого, может быть, я убедлю вас, что, как бы ни была плоха ситуация, могло быть и хуже. ВСЕГДА может быть хуже. :-)

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

Код Отзывы

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

Когда мы слились, все было плохо. Новые функции были на 6-12 месяцев позже, год за годом. Журнал ожидания был огромным, растущим и истощающим жизнь. Кодекс владения был Сильный, особенно среди самых старших "гуру". «Художественные ветки» иногда длились годами и охватывали несколько релизов; иногда НИКТО, но один разработчик видел код до того, как он попал в основную ветку. На самом деле «функциональная ветвь» вводит в заблуждение, поскольку предполагает, что код был где-то в хранилище. Чаще всего это было только на индивидуальной системе разработчика.

Руководство согласилось с тем, что нам нужно «что-то сделать», прежде чем качество станет неприемлемо низким. :-) Их ответ был Code Reviews. Code Reviews стал официальным пунктом «Ты будешь», который предшествовал каждой регистрации. Инструмент, который мы использовали, был Review Board.

Как это работает в культуре, которую я описал? Лучше, чем ничего, но это было больно, и потребовалось больше года, чтобы достичь своего рода минимального уровня соответствия. Некоторые вещи, которые мы наблюдали:

  1. Инструмент, который вы используете, имеет тенденцию фокусировать обзоры кода определенным образом. Это может быть проблемой. Совет по рассмотрению предоставляет вам красивые, красочные построчные различия и позволяет добавлять комментарии к строке. Это заставляет большинство разработчиков игнорировать все строки, которые не изменились. Это хорошо для небольших, изолированных изменений. Это не очень хорошо для больших изменений, больших кусков нового кода или кода, который имеет 500 экземпляров переименованной функции, смешанной с 10 строками новой функциональности.
  2. Даже при том, что мы были в старой, больной кодовой базе, которая в основном никогда не проверялась раньше, рецензенту стало «невежливо» комментировать все, что НЕ было строкой изменений, даже указывать на очевидную ошибку. «Не беспокойте меня, это важная регистрация, и у меня нет времени, чтобы исправить ошибки».
  3. Магазин для "простого" рецензента. Некоторые люди просматривают 10-файловый обзор с 2000 измененными строками в течение 3 минут и нажимают кнопку «Отправь!». Все быстро узнают, кто эти люди. Если вы действительно не хотите, чтобы ваш код был проверен в первую очередь, отправьте его «простому» рецензенту. Ваша регистрация не будет замедлена. Вы можете вернуть услугу, став «легким» рецензентом своего кода.
  4. Если вы ненавидите обзоры кода, просто игнорируйте электронные письма, которые вы получаете от Review Board. Не обращайте внимания на последующие действия от членов вашей команды. Неделями. Пока у вас не будет 3 дюжины открытых отзывов в очереди, и ваше имя будет появляться на групповых собраниях несколько раз. Тогда станьте «легким» рецензентом и очистите все ваши отзывы до обеда.
  5. Старайтесь не посылать свой код «жесткому» рецензенту. (Разработчик, который будет задавать или отвечать на вопросы, подобные этому.) Все быстро узнают, кто такие «жесткие» рецензенты, так же, как они изучают простых. Если проверки кода - это растрата времени (™), то чтение подробного отзыва о СВОЕМ коде - это и растрата времени (™), и оскорбление.
  6. Когда проверки кода являются болезненными, люди откладывают их и продолжают писать больше кода. Вы получаете меньше обзоров кода, но каждый из них БОЛЬШОЙ. Вам нужно больше меньших обзоров кода, а это значит, что команде нужно выяснить, как сделать их как можно более безболезненными.

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

Рефакторинг

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

Но код по-прежнему остро нуждался в рефакторинге. Как продолжить?

  1. Никогда не смешивайте изменение рефакторинга с функциональным изменением. Ряд людей упомянули об этом. Если проверки кода - это проблема, не увеличивайте ее. Это больше работы, но вы должны запланировать отдельный обзор и отдельную регистрацию.
  2. Начните с малого. Очень маленький. Даже меньше, чем это. Используйте очень маленькие рефакторинги, чтобы постепенно научить людей, что рефакторинг и обзоры кода не являются чистым злом. Если вы можете прокрасться одним крошечным рефакторингом в неделю без особой боли, через пару месяцев вы сможете сойти с рук с двумя в неделю. И они могут быть немного больше. Лучше чем ничего.
  3. У нас практически не было модульных тестов. Так что рефакторинг запрещен, верно? Не обязательно. Для некоторых изменений компилятор является вашим модульным тестом. Сосредоточьтесь на рефакторингах, которые вы можете проверить. Избегайте изменений, если они не могут быть проверены. Возможно, вместо этого потратьте время на написание юнит-тестов.
  4. Некоторые разработчики боятся рефакторинга, потому что боятся ВСЕХ изменений кода. Мне потребовалось много времени, чтобы распознать этот тип. Напишите кусок кода, возитесь с ним, пока он не «заработает», и НИКОГДА не хотите его менять. Они не обязательно понимают, ПОЧЕМУ это работает. Изменения рискованны. Они не доверяют себе вносить ЛЮБЫЕ изменения и, конечно же, не будут доверять вам. Предполагается, что рефакторинг - это небольшие, безопасные изменения, которые не меняют поведение. Есть разработчики, для которых сама идея «изменений, которые не меняют поведение» немыслима . Я не знаю, что предложить в этих случаях. Я думаю, что вы должны пытаться работать в областях, которые им не нужны. Я был удивлен, когда узнал, что этот тип может иметь долго,
GraniteRobert
источник
1
Это супер продуманный ответ, спасибо! Я особенно согласен с тем, как инструмент CR может повлиять на фокус обзора ... построчно - это простой выход, который не предполагает реального понимания того, что происходило раньше и что происходит сейчас. И, конечно, код, который не изменился, совершенен, нет необходимости смотреть на это ...
t0x1n
1
« Может быть хуже. Может быть дождь». Когда я прочитал ваш последний абзац (второй пункт № 4), я подумал: им нужно больше рецензировать, рецензировать кодер . И некоторый рефакторинг, как в: "yourSalary = 0"
Абсолютно точно на стольких фронтах, невероятный ответ. Я могу полностью видеть, откуда вы: я сам в той же ситуации, и это невероятно расстраивает. Вы находитесь в этой постоянной борьбе за качество и передовой опыт, и поддержка со стороны не только руководства, но и ОСОБЕННО других разработчиков в команде.
Jullealgon
13

Почему бы вам не сделать оба, но с отдельными коммитами?

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

Но если вы видите ряд вопиющих проблем, есть два варианта, которым вы можете следовать.

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

  2. Если при проверке кода возникли проблемы, требующие исправления, попросите разработчика провести рефакторинг на основе рекомендаций Resharper.


источник
Я понял из твоего ответа, что ты веришь в сильное владение кодом. Я призываю вас прочитать мысли Фаулера о том, почему это плохая идея: martinfowler.com/bliki/CodeOwnership.html . Если говорить конкретно о ваших пунктах: (1) это миф, так как рефакторинг происходит во время внесения изменений, а не до или после отдельным, чистым, несвязанным способом, как того требуют отдельные CR. (2) С большинством разработчиков этого никогда не случится. Никогда . Большинство разработчиков не заботятся об этих вещах, тем более, если они приходят от какого-то другого парня, который не является их менеджером. У них есть свои собственные вещи, которые они хотят сделать.
t0x1n
8
@ t0x1n: Если ваши менеджеры не заботятся о рефакторинге, а ваши коллеги-разработчики не заботятся о рефакторинге ... тогда эта компания медленно тонет. Убегать! :)
logc
8
@ t0x1n только потому, что вы вносите изменения вместе, не означает, что вы должны фиксировать их вместе. Кроме того, часто полезно проверить, что у вашего рефакторинга не было неожиданных побочных эффектов отдельно от проверки того, что ваши функциональные изменения имели ожидаемый эффект. Очевидно, что это может относиться не ко всем рефакторингу, но в целом это неплохой совет.
бесполезно
2
@ t0x1n - я не помню, чтобы что-то говорил о владении сильным кодом. Мой ответ состоял в том, чтобы сохранить вашу роль рецензента в чистоте. Если рецензент вносит изменения, они больше не являются рецензентом.
3
@ GlenH7 Возможно, здесь было какое-то недопонимание - я не рецензент. Я просто кодирую то, что мне нужно, и сталкиваюсь с кодом, который я могу улучшить в процессе. Мои рецензенты тогда жалуются, когда я делаю.
t0x1n
7

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

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

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

Кроме того, предполагая, что вы выполняете TDD или какой-либо другой красно-зеленый рефакторинг, один из способов обеспечить взаимодействие со сверстниками - это использовать метод сопряжения в пинг-понге. Проще говоря, что драйвер вращается для каждого шага цикла RGR, то есть пара 1 записывает неудачный тест, пара 2 исправляет его, пара 1 выполняет рефакторинг, пара 2 пишет неудачный тест .... и так далее.

daffers
источник
Отличные баллы. К сожалению, я искренне сомневаюсь, что смогу изменить «систему». Иногда рецензенты тоже из разных часовых поясов и географических местоположений, поэтому в таких случаях он не будет летать независимо.
t0x1n
6

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

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

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

AlfredoCasado
источник
1
Вы начинаете хотеть выиграть оценочную игру, как только понимаете, что она вознаграждает вас повышением зарплаты, бонусами и опционами на акции :)
t0x1n
2
Нет. Вы торгуете деньгами по принципам @ t0x1n. Просто как тот. У вас есть три варианта: поработать над исправлением системы, принять систему, выйти из системы. Варианты 1 и 3 хороши для души.
Шон Перри
2
не только плохо для души, компания с принципами, ориентированными на локальные максимумы, обычно менее оптимальна, чем компания, ориентирующаяся на глобальные максимы. Не только это, работа это не только деньги, вы тратите много времени каждый день на работе, чувствуете себя комфортно на работе, чувствуете, что вы делаете правильные вещи, возможно, это намного лучше, чем собирать больше долларов каждый месяц.
AlfredoCasado
1
Мех, души переоценены;)
t0x1n
2
@SeanPerry Я действительно пытался исправить систему, но это было очень сложно. (1) Я был практически одинок в этом, и мне трудно идти против больших вождей (я просто обычный разработчик, даже не старший). (2) Эти вещи занимают время, которого у меня просто не было. Работы много, и вся среда отнимает много времени (электронная почта, перерывы, CR, неудачные циклы тестирования, которые нужно исправить, встречи и т. Д. И т. Д.). Я делаю все возможное, чтобы фильтровать и быть продуктивным, но обычно я едва могу закончить свою «предписанную» работу вовремя (высокие стандарты здесь не помогают), не говоря уже о работе по изменению системы ...
t0x1n
5

Иногда рефакторинг - это плохо. Не по тем причинам, которые дают ваши обозреватели кода; это звучит , как они на самом деле не заботиться о качестве кода, и вы делаете уход, который является удивительным. Но есть две причины, которые должны помешать вам выполнить рефакторинг : 1. Вы не можете тестировать изменения, сделанные вами с помощью автоматических тестов (модульные тесты или что-то еще), или 2. Вы вносите изменения в некоторый код, который вы не очень понимаете. Что ж; то есть, вы не обладаете знанием предметной области, чтобы знать, какие изменения вы должны внести.

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

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

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

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

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

Вместо того, чтобы переименовывать идиоматические переменные, просто поместите комментарии, объясняющие, что они есть. Если они не идиоматичны, пожалуйста, переименуйте их. Не оставляйте i, j, k, x, yи т.д. плавающие вокруг , когда лучшие имена будут работать.

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

У нас есть следующие понятия, которые мы всегда сокращаем, когда говорим о них: «Зона концерна» становится «AOC», «Взрыв парового облака» становится VCE, и тому подобное. В нашей кодовой базе кто-то реорганизовал все экземпляры, называемые aoc, в AreaOfConcern, VCE вaporCloudExplosion, nPlanes в confiningPlanesCount ... что на самом деле затруднило чтение кода для людей, обладающих знаниями в конкретной области. Я тоже был виновен в подобных вещах.


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

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

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

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

Как уже говорилось в других ответах - можете ли вы отделить проверки рефакторинга от изменений кода (не обязательно в качестве отдельных обзоров)? В зависимости от того, какой инструмент вы используете для проверки кода, вы сможете просматривать различия только между конкретными ревизиями (Atlassian's Crucible определенно делает это).

(2) Никто не будет благосклонно смотреть на то, как вы выпускаете «рефакторинг» КР, которые вы не должны были делать. CR имеет определенные накладные расходы, и ваш менеджер не хочет, чтобы вы «тратили время» на рефакторинг. Когда это связано с изменением, которое вы должны сделать, эта проблема сводится к минимуму.

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

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

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

Крис Купер
источник
Что касается разделения CR, я указал в своем посте и нескольких других комментариях, почему я считаю это невозможным.
t0x1n
Я не говорю о придирчивых вещах, я говорю о фактических проблемах производительности и корректности, а также о избыточном коде, дублирующем коде и т. Д. И это только материал R #, есть также действительно плохой код, который я легко могу исправить , К сожалению, не вся моя команда использует reharper, и даже те, кто не воспринимает это слишком серьезно. Необходимы огромные образовательные усилия, и, возможно, я постараюсь привести что-то подобное. Это тяжело, потому что у меня едва хватает времени на работу , не говоря уже об образовательных проектах. Возможно, мне просто нужно дождаться периода простоя, чтобы использовать его.
t0x1n
Я просто собираюсь вмешаться и сказать, что это определенно не невозможно , поскольку я вижу, что это делается все время. Внесите изменения, которые вы сделаете, без рефакторинга, зарегистрируйте их, затем выполните рефакторинг, чтобы очистить, и проверьте это. Это не ракетостроение. Да, и будьте готовы отстаивать, почему вы считаете целесообразным потратить время на рефакторинг и проверку переработанного кода.
Эрик Кинг,
@EricKing Я полагаю, я мог бы сделать это. Однако: (1) мне придется работать с уродливым кодом и вести записи о том, что я хочу улучшить, пока я не закончу с функциональными изменениями, которые одновременно отстой и фактически замедляют мой функциональный прогресс (2) как только я отправлю свои функциональные изменения и пересмотреть мои заметки по рефакторингу, это будет только первая итерация, и для завершения рефакторинга может потребоваться больше времени, что, как вы предположили, мне будет сложно объяснить моим менеджерам, поскольку моя работа уже «выполнена».
t0x1n
2
«Я говорю о реальных проблемах с производительностью и корректностью» - тогда это может растянуть определение рефакторинга; если код на самом деле неверен, это будет означать исправление ошибки. Что касается проблем с производительностью, это не то, что вы должны просто исправлять как часть изменения функциональности, это, вероятно, то, что требует измерения, тщательного тестирования и отдельного анализа кода.
Крис Купер
2

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

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

Фил Перри
источник
Да, у меня тоже есть шансы на расстояние, а также на такие мелочи, как избыточное приведение и т. Д. Все, что не полностью обусловлено моими изменениями, - это «шум».
t0x1n
2

Если вы можете разделить запрошенное изменение и незапрошенный рефакторинг на (множество) отдельных коммитов, как отметили @Useless, @Telastyn и другие, то это лучшее, что вы можете сделать. Вы по-прежнему будете работать над одним CR, без административных накладных расходов на создание «рефакторинга». Просто держите ваши коммиты маленькими и сфокусированными.

Вместо того, чтобы дать вам несколько советов, как это сделать, я предпочитаю указать вам гораздо большее объяснение (фактически, книгу) от специалиста. Таким образом, вы сможете узнать намного больше. Прочитайте Работая эффективно с Устаревшим Кодексом (Майклом Фезерсом) . Эта книга может научить вас, как сделать рефакторинг, чередующийся с функциональными изменениями.

Темы включают в себя:

  • Понимание механизма изменения программного обеспечения: добавление функций, исправление ошибок, улучшение дизайна, оптимизация производительности
  • Получение унаследованного кода в тестовом жгуте
  • Написание тестов, которые защитят вас от новых проблем
  • Методы, которые можно использовать с любым языком или платформой - с примерами на Java, C ++, C и C #
  • Точная идентификация того, где необходимо внести изменения в код
  • Справиться с устаревшими системами, которые не являются объектно-ориентированными
  • Обработка приложений, которые не имеют какой-либо структуры

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

Hbas
источник
2

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

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

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

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

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

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

Марио Т. Ланца
источник
1

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

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

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

logc
источник
Это было бы хорошим предложением для нового проекта. Однако наша текущая кодовая база огромна, и Resharper выдает много ошибок для большинства файлов. Просто слишком поздно применять его, а подавление существующих ошибок делает его еще хуже - вы пропустите их в своем новом коде. Также есть много ошибок, проблем и запахов кода, которые статический анализатор не уловит, я просто приводил предупреждения Решарпера в качестве примера. Опять же, я мог бы сформулировать «напрасную трату» слишком резко, я должен был сказать что-то вроде «посвященного времени чему-то, что не является приоритетом».
t0x1n
@ t0x1n: правило бойскаута включает модули, к которым вы прикоснулись, в основном. Это может помочь вам провести первую разделительную линию. Подавлять предупреждения - не очень хорошая идея, я знаю, но с точки зрения компании, подавлять их в новом коде правильно и следовать их правилам - ну, может быть, я увлекаюсь своей собственной аргументацией :)
logc
1
это расстраивающая часть! Я касаюсь только тех файлов, которые я бы касался в любом случае как часть моей задачи, но я получаю жалобы точно так же! Предупреждения, о которых я говорю, не являются предупреждением о стиле, я говорю о фактических проблемах производительности и корректности, а также о избыточном коде, дублированном коде и т. Д.
t0x1n
@ t0x1n: это звучит очень расстраивающе. Обратите внимание, что я имел в виду не только «статический анализ кода», но и другие рекомендации, что-то эквивалентное Nexus. Конечно, ни один инструмент не улавливает 100% семантику; это всего лишь стратегия исправления.
журнал