Это запах кода, чтобы установить флаг в цикле, чтобы использовать его позже?

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

Пример:

Map<BigInteger, List<String>> map = handler.getMap();

if(map != null && !map.isEmpty())
{
    for (Map.Entry<BigInteger, List<String>> entry : map.entrySet())
    {
        fillUpList();

        if(list.size() > limit)
        {
            limitFlag = true;
            break;
        }
    }
}
else
{
    logger.info("\n>>>>> \n\t 6.1 NO entries to iterate over (for given FC and target) \n");
}

if(!limitFlag) // Continue only if limitFlag is not set
{
    // Do something
}

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

Я прав? Как я мог удалить это?

Нет ничего плохого в использовании логического значения по назначению: для записи двоичного различия.

Если бы мне сказали реорганизовать этот код, я бы, вероятно, поместил цикл в собственный метод, чтобы присваивание + breakпревращалось в a return; тогда вам даже не нужна переменная, вы можете просто сказать

if(fill_list_from_map()) {
  ...

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

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

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

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

«Кодовые запахи» - это когда вы не думаете. Если вы действительно собираетесь думать о коде, делайте это правильно!

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

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

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

Да, это запах кода (реплики от всех, кто это делает).

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

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

Когда код так же прост, как ваш пример, он может быть сведен к whileциклу или эквивалентной карте, фильтруемой конструкции.

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

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

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

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

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

Если это нарушает ваш код, почему?

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

private <T> boolean checkCollection(Collection<T> collection)
{
    for (T element : collection)
        if (checkElement(element))
            return true;
    return false;
}

Начиная с Java 8, есть более сжатый способ использования Stream.anyMatch(…):

collection.stream().anyMatch(this::checkElement);

В вашем случае это, вероятно, будет выглядеть так (при условии, что list == entry.getValue()в вашем вопросе):

map.values().stream().anyMatch(list -> list.size() > limit);

Проблема в вашем конкретном примере - дополнительный вызов fillUpList(). Ответ во многом зависит от того, что этот метод должен делать.

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

Поэтому я предполагаю, что фактический код передает текущий entryметод методу.

Но есть еще вопросы, чтобы задать:

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

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

Это может означать следующее («все или ничего», а достижение предела указывает на ошибку):

/**
 * Computes the list of all foo strings for each passed number.
 * 
 * @param numbers the numbers to process. Must not be {@code null}.
 * @return all foo strings for each passed number. Never {@code null}.
 * @throws InvalidArgumentException if any number produces a list that is too long.
 */
public Map<BigInteger, List<String>> computeFoos(Set<BigInteger> numbers)
        throws InvalidArgumentException
{
    if (numbers.isEmpty())
    {
        // Do you actually need to log this here?
        // The caller might know better what to do in this case...
        logger.info("Nothing to compute");
    }
    return numbers.stream().collect(Collectors.toMap(
            number -> number,
            number -> computeListForNumber(number)));
}

private List<String> computeListForNumber(BigInteger number)
        throws InvalidArgumentException
{
    // compute the list and throw an exception if the limit is exceeded.
}

Или это может означать это («обновить до первой проблемы»):

/**
 * Refreshes all foo lists after they have become invalid because of bar.
 * 
 * @param map the numbers with all their current values.
 *            The values in this map will be modified.
 *            Must not be {@code null}.
 * @throws InvalidArgumentException if any new foo list would become too long.
 *             Some other lists may have already been updated.
 */
public void updateFoos(Map<BigInteger, List<String>> map)
        throws InvalidArgumentException
{
    map.replaceAll(this::computeUpdatedList);
}

private List<String> computeUpdatedList(
        BigInteger number, List<String> currentValues)
        throws InvalidArgumentException
{
    // compute the new list and throw an exception if the limit is exceeded.
}

Или это («обновить все списки, но сохранить оригинальный список, если он становится слишком большим»):

/**
 * Refreshes all foo lists after they have become invalid because of bar.
 * Lists that would become too large will not be updated.
 * 
 * @param map the numbers with all their current values.
 *            The values in this map will be modified.
 *            Must not be {@code null}.
 * @return {@code true} if all updates have been successful,
 *         {@code false} if one or more elements have been skipped
 *         because the foo list size limit has been reached.
 */
public boolean updateFoos(Map<BigInteger, List<String>> map)
{
    boolean allUpdatesSuccessful = true;
    for (Entry<BigInteger, List<String>> entry : map.entrySet())
    {
        List<String> newList = computeListForNumber(entry.getKey());
        if (newList.size() > limit)
            allUpdatesSuccessful = false;
        else
            entry.setValue(newList);
    }
    return allUpdatesSuccessful;
}

private List<String> computeListForNumber(BigInteger number)
{
    // compute the new list
}

Или даже следующее (используя computeFoos(…)первый пример, но без исключений):

/**
 * Processes the passed numbers. An optimized algorithm will be used if any number
 * produces a foo list of a size that justifies the additional overhead.
 * 
 * @param numbers the numbers to process. Must not be {@code null}.
 */
public void process(Collection<BigInteger> numbers)
{
    Map<BigInteger, List<String>> map = computeFoos(numbers);
    if (isLimitReached(map))
        processLarge(map);
    else
        processSmall(map);
}

private boolean isLimitReached(Map<BigInteger, List<String>> map)
{
    return map.values().stream().anyMatch(list -> list.size() > limit);
}

Или это может означать что-то совершенно другое… ;-)