Должен ли я реорганизовать код, который отмечен как «не изменения»?

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

Во время рефакторинга время от времени я сталкиваюсь с классом, методом или строками кода, которые имеют такие комментарии, как

  

Тайм-аут установлен, чтобы дать модуль A некоторое время, чтобы сделать что-то. Если это не так, как это, он сломается.

или

  

Не меняйте это. Поверь мне, ты сломаешь вещи.

или

  

Я знаю, что использование setTimeout не является хорошей практикой, но в этом случае мне пришлось использовать его

Мой вопрос: должен ли я реорганизовать код, когда я сталкиваюсь с такими предупреждениями от авторов (нет, я не могу связаться с авторами)?

144 голоса | спросил kukis 5 PMpWed, 05 Apr 2017 17:03:12 +030003Wednesday 2017, 17:03:12

13 ответов


221

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

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

Вот лучшая стратегия: используйте свое время для

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

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

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

  • получите копию книги Пера «Эффективная работа с устаревшим кодом» для команды.

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

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

ответил Doc Brown 5 PMpWed, 05 Apr 2017 17:24:47 +030024Wednesday 2017, 17:24:47
141

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

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

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

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

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

ответил Kilian Foth 5 PMpWed, 05 Apr 2017 17:20:04 +030020Wednesday 2017, 17:20:04
61
  

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

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

  

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

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

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

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

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

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

Возможно, получите копию «Эффективная работа с устаревшим кодом»?

  

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

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

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

ответил Daenyth 5 PMpWed, 05 Apr 2017 17:22:57 +030022Wednesday 2017, 17:22:57
23

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

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

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

До этого я не касался кода, который помечен как «не трогать».

ответил 9000 5 PMpWed, 05 Apr 2017 17:25:09 +030025Wednesday 2017, 17:25:09
6

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

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

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

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

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

ответил cmaster 5 PMpWed, 05 Apr 2017 19:31:11 +030031Wednesday 2017, 19:31:11
6

Я говорю, иди и измени его. Верьте или нет, не каждый кодер является гением, и предупреждение, подобное этому, означает, что это может быть точкой для улучшения. Если вы обнаружите, что автор прав, вы можете (вздохнуть) ДОКУМЕНТ или ОБЪЯСНИТЬ причину предупреждения.

ответил JES 6 PMpThu, 06 Apr 2017 16:58:13 +030058Thursday 2017, 16:58:13
5

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

Но если вы должны рефакторинг ...

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

ответил FrustratedWithFormsDesigner 5 PMpWed, 05 Apr 2017 17:19:48 +030019Wednesday 2017, 17:19:48
4

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

В этот момент вопрос о том, нужно ли рефактору, преждевременен (хотя на него, вероятно, будет отвечать конкретная форма «да»).

Центральная проблема здесь в том, что (как отмечалось в некоторых ответах) комментарии, которые вы цитируете, сильно указывают на то, что код имеет условия гонки или другие проблемы параллелизма /синхронизации, например обсуждаемые здесь . Это особенно сложные проблемы по нескольким причинам. Во-первых, как вы обнаружили, казалось бы, несвязанные изменения могут вызвать проблему (другие ошибки также могут иметь этот эффект, но ошибки параллелизма почти всегда делают.) Во-вторых, их очень сложно диагностировать: ошибка часто проявляется в месте, которое отдаленный во времени или код от причины, и все, что вы делаете для его диагностики, может привести к его исчезновению ( Heisenbugs ). В-третьих, ошибки параллелизма очень трудно найти в тестировании. Отчасти это связано с комбинаторным взрывом: оно достаточно плохо для последовательного кода, но добавление возможных перемежений параллельного выполнения приводит к тому, что последовательная проблема становится незначительной в сравнении. Кроме того, даже хороший тестовый случай может только вызвать проблему изредка - Нэнси Левезон подсчитала, что одна из смертельных ошибок в Therac 25 произошел в 1 из примерно 350 прогонов, но если вы не знаете, что такое ошибка, или даже что есть, вы не знаете, сколько повторений делает эффективный тест. Кроме того, в этом масштабе возможно только автоматическое тестирование, и возможно, что тестовый драйвер накладывает тонкие ограничения по времени таким образом, что он никогда не вызовет ошибку (Heisenbugs снова).

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

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

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

ответил sdenham 7 PMpFri, 07 Apr 2017 15:34:18 +030034Friday 2017, 15:34:18
3
  

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

     

Во время рефакторинга время от времени я сталкиваюсь с классом, методом или строками кода, которые имеют такие комментарии, как ["не трогайте это!"]

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

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

ответил DepressedDaniel 9 AMpSun, 09 Apr 2017 00:57:11 +030057Sunday 2017, 00:57:11
2

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

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

В идеале вы сможете выяснить, в чем проблема, и исправить ее правильно. Например:

  

Тайм-аут установлен, чтобы дать модуль A некоторое время, чтобы сделать что-то. Если это не так, как это, он сломается.

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

  

Не меняйте это. Поверь мне, ты сломаешь вещи.

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

  

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

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

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

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

ответил David Schwartz 6 PMpThu, 06 Apr 2017 17:30:36 +030030Thursday 2017, 17:30:36
2

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

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

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

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

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

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

ответил Thomas Carlisle 7 PMpFri, 07 Apr 2017 20:24:13 +030024Friday 2017, 20:24:13
1

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

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

Это означает:

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

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

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

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

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

ответил JacquesB 12 AMpWed, 12 Apr 2017 11:21:20 +030021Wednesday 2017, 11:21:20
-1

Вопрос, который я задал бы, - это то, почему кто-то написал «НЕ РЕДАКТИРУЙТЕ» в первую очередь.

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

Держу пари, в этом случае это произошло, и человек, написавший комментарий, обнаружил, что кто-то изменил его, а затем пришлось повторить все упражнение и вернуть его так, как надо, чтобы заставить дело работать. После этого из-за сдвиговых разочарований они писали ... НЕ ИЗМЕНИТЬ.

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

Говоря «НЕ ИЗМЕНИЙ», это способ сказать, что мы знаем все, что мы когда-либо будем знать прямо сейчас, и поэтому в будущем мы никогда не узнаем ничего нового.

Должен быть хотя бы комментарий о причинах не редактирования. Например, «Не прикасаться» или «Не вводить». Почему бы не прикоснуться к забору? Почему бы не войти? Не лучше ли сказать: «Электрический забор, не трогай!» или «Земельные шахты! Не вводите!». Тогда очевидно, почему, и все же вы все еще можете войти, но, по крайней мере, знаете последствия, прежде чем делать.

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

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

ответил BigA 7 AMpFri, 07 Apr 2017 08:33:49 +030033Friday 2017, 08:33:49

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132