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

59

Мне было поручено увеличить охват кода существующего проекта Java.

Я заметил, что инструмент покрытия кода ( EclEmma ) выделил некоторые методы, которые никогда нигде не вызываются.

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

Какой будет лучший подход? Напишите для них юнит-тесты или вопрос, почему они там?

Лукас Т
источник
1
Комментарии не для расширенного обсуждения; этот разговор был перенесен в чат .
maple_shaft

Ответы:

118
  1. Удалять.
  2. Commit.
  3. Забудьте.

Обоснование:

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

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

l0b0
источник
118
Обычно я согласен с таким подходом, но если вы новичок в проекте и / или младший разработчик, лучше спросите своего наставника / начальника перед удалением. В зависимости от проекта некоторые методы могут выглядеть не используемыми, а вызываться / использоваться внешним кодом, не входящим в код проекта, к которому у вас есть доступ. Это может быть автоматически сгенерированный код, поэтому ваше удаление ничего не даст, кроме шума журнала. Возможно, ваша IDE не сможет найти доступ на основе отражения изнутри проекта. И т.д. с. Если вы не уверены в таких угловых делах, спросите хотя бы один раз.
Фрэнк Хопкинс
11
Хотя я согласен с сутью этого ответа, буквальный вопрос был «должен ли я задаться вопросом, почему они там?» , Этот ответ может создать впечатление, что OP не должен спрашивать свою команду перед удалением.
Док Браун
58
Хотя сначала я добавлю шаг - проверим журнал git blame на наличие неиспользуемых строк кода. Если вы можете найти человека, который написал их, вы можете спросить, для чего они. Если линии новые, есть большая вероятность, что кто-то планирует использовать их в ближайшее время (в этом случае они, вероятно, должны быть проверены модулем сейчас).
BDSL
10
В этом ответе так много неправильного, выраженного в комментариях или других ответах, что я не могу поверить, что это самый популярный и принятый ответ.
user949300
11
@Emory Лучший способ начать работу в новой команде - сломать сборку, небрежно удаляя вещи, потому что вы думали, что они никому не нужны. Гарантирует, что вы популярны с первого дня. Конечно, это может и не понадобиться (потому что каждое большое старое приложение всегда имеет 100% -ный кашель с покрытием кода ), но это очень плохая рентабельность инвестиций.
Во
55

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

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

В этом случае есть четыре возможности:

  • Если методы являются частными или пакетными, их можно безопасно удалить.
  • Если методы общедоступны, их наличие может быть оправдано даже без фактического использования для полноты функциональности. Они должны быть проверены, хотя.
  • Если методы общедоступны и не нужны, их удаление будет серьезным изменением, и если библиотека следует семантическому версионированию , это разрешено только в новой основной версии.
  • В качестве альтернативы, публичные методы также могут быть устаревшими и удалены позже. Это дает потребителям API некоторое время для перехода от устаревших функций до того, как они будут удалены в следующей основной версии.
Zoltan
источник
9
Плюс, если это библиотека, функции существуют для полноты
картины
Можете ли вы уточнить, как они должны быть проверены? Если вы не знаете, почему этот метод существует, вы, вероятно, не знаете, что он должен делать.
Нет U
Имена методов, такие как filter_name_exists, ReloadSettingsили addToSchema(случайно выбранные из 3 произвольных проектов с открытым исходным кодом) должны предоставить несколько советов о том, что метод должен делать. Комментарий Javadoc может быть еще более полезным. Я знаю, что это неправильная спецификация, но этого может быть достаточно, чтобы создать несколько тестов, которые, по крайней мере, могут предотвратить регрессию.
Золтан
1
публичные методы в закрытом классе или интерфейсе не должны считаться публичными для этой цели. Точно так же, если публичный класс вложен в частный класс, он не является публичным.
Эмори
30

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

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

Ewan
источник
Спасибо, сделаю. В Eclipse я выполняю поиск по базе кода для функции, и ничего не происходит. Если у вас есть другие предложения о том, как сделать более полный поиск, я был бы очень признателен.
Лукас Т
3
да, вы должны проверить другие проекты, которые могут импортировать класс как dll
Ewan
7
Этот ответ кажется неполным. «Сначала убедитесь, что ваш инструмент покрытия кода корректен. Если это так, тогда .... [вставьте остаток ответа] ». В частности, ОП хочет знать, что делать, если инструмент покрытия кода правильный.
Джон Бентли
1
@jonbentley ОП просит лучшего подхода к отчету об инструментах. «Проверяй это вручную», потому что из контекста довольно очевидно, что это неверно
Эван
15

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

max630
источник
9
+1 для проверки с людьми, это своего рода принцип наименьшего удивления. Вы не хотите, чтобы кто-то проводил слишком много времени в поисках метода, оставленного там несколькими коммитами ранее. Кроме того, в некоторых крайних случаях мертвый код может быть новым материалом, который уже зарегистрирован, но еще нигде не подключен (хотя в этом случае он должен быть хорошо задокументирован с комментариями и протестирован).
Frax
4
Хорошо, если код не использует отражение для вызова «неиспользуемых» методов, но в этом случае у вас гораздо большие проблемы, и мы с нетерпением ожидаем увидеть код на thefailywft.com (Проведите быстрый тест для определения, если какой-либо код выполняет ClassName. класс).
MTilsted
2
Он статически скомпилирован, но есть и invokedynamicинструкция, так что, вы знаете ...
CorsiKa
@MTilsted: Есть много фреймворков, которые будут вызывать код с помощью строк - я могу думать о Spring и Hibernate, хотя бы на пределе.
1818 года
9

Какой будет лучший подход? Напишите для них юнит-тесты или вопрос, почему они там?

Удаление кода это хорошая вещь.

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

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

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

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

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

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

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

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

вопрос почему они там?

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

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

VoiceOfUnreason
источник
9

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

Если вы не уверены, ставьте assert () в начале каждого «неиспользуемого» метода. Может быть, это называется. Или заявление о регистрации.

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

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

Интересно, что худший из возможных советов «Удалить. Фиксация. Забудь». это самый высокий рейтинг. (Обзоры кода? Вы не делаете обзоры кода? Что на самом деле вы делаете программирование, если вы не делаете обзоры кода?)

gnasher729
источник
Я с уважением не согласен с тем, что «УДАЛИТЬ. КОМИТ. ЗАБЫТЬ» - худший совет. (Я думаю, что это лучшее.) Ваш подход тоже в порядке. Я думаю, что худшим из возможных советов было бы написать модульные тесты, которые используют «мертвый код», но не делают никаких утверждений. Инструмент покрытия кода будет обманут, думая, что он используется.
Эмори
3
Ничто в ответе «Удалить. Фиксация. Забудь» не говорит о том, что не нужно проверять код. Вполне возможно (и желательно, ИМХО) просмотреть код после его фиксации (но перед развертыванием :-)).
слеське
@sleske Как вы просматриваете код, которого больше нет? В этом ответе также не упоминается «обзор».
user949300
@ user949300: Вопрос о том, нужно ли пересматривать изменение кода или нет, является отдельным вопросом, и не зависит от того, был ли код добавлен, изменен или удален (добавление кода может быть даже более катастрофическим, чем его удаление, см., например, уязвимость Heartbleed). Кроме того, в ответе (сейчас) написано «привлечь эксперта», что звучит довольно близко к обзору кода.
слеське
1
@ user949300: Что касается «Как вы просматриваете код, которого больше нет» - я надеюсь, что это очевидно: если посмотреть на изменения в выбранном вами инструменте контроля версий. Как еще вы будете просматривать изменения (любые изменения)?
слеске
6

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

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

Мы регистрируемся на выделенном #unreachable-codeрезервном канале с уникальным идентификатором для каждого кандидата на удаление, и это оказалось довольно эффективным.

Джейми Булл
источник
Но что, если его редко называют (например, 1 на 1440 в среднем) ?
Питер Мортенсен
тогда «подходящий период времени» длиннее (хотя мне нравится «человек после полуночи»)
Джейми Булл,