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

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

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

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

или же

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

или же

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

До этого я бы не трогал код с надписью «не трогай».

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

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

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

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

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

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

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

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

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

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

На данный момент вопрос о том, следует ли проводить рефакторинг, является преждевременным (даже если на него, вероятно, ответит конкретная форма «да»).

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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