Is it a code smell to set a flag in a loop to use it later?

Question

I have a piece of code where I iterate a map until a certain condition is true and then later on use that condition to do some more stuff.

Example:

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
}

I feel setting a flag and then using that to do more stuff is a code smell.

Am I right? How could I remove this?

Answer 1

There's nothing wrong with using a Boolean value for its intended purpose: to record a binary distinction.

If I were told to refactor this code, I'd probably put the loop into a method of its own so that the assignment + break turns into a return; then you don't even need a variable, you can simply say

if(fill_list_from_map()) {
  ...

Answer 2

It is not necessarily bad, and sometimes it is the best solution. But setting flags like this in nested blocks can make code hard to follow.

The problem is you have blocks to delimit scopes, but then you have flags which communicate across scopes, breaking the logical isolation of the blocks. For example, the limitFlag will be false if the map is null, so the "do something"-code will be executed if map is null. This may be what you intend, but it could be a bug which is easy to miss, because the conditions for this flag is defined somewhere else, inside a nested scope. If you can keep information and logic inside the tightest possible scope, you should attempt to do so.

Answer 3

I'd advise against reasoning about 'code smells'. That's just the laziest possible way to rationalize your own biases. Over time you'll develop a lot of biases, and a lot of them will be reasonable, but a lot of them will be stupid.

Instead, you should have practical (i.e., not dogmatic) reasons for preferring one thing over another, and avoid thinking that you should have the same answer for all similar questions.

"Code smells" are for when you aren't thinking. If you're really going to think about the code, then do it right!

In this case, the decision could really go either way depending on the surrounding code. It really depends on what you think is the clearest way to think about what the code is doing. ("clean" code is code that clearly communicates what it's doing to other developers and makes it easy for them to verify that it is correct)

A lot of times, people will write methods structured into phases, where the code will first determine what it needs to know about the data and then act on it. If the "determine" part and the "act on it" part are both a little complicated, then it can make good sense to do this, and often the "what it needs to know" can be carried between phases in Boolean flags. I would really prefer that you gave the flag a better name, though. Something like "largeEntryExists" would make the code a lot cleaner.

If, on the other hand, the "// Do Something" code is very simple, then it can make more sense to put it inside the if block instead of setting a flag. That puts the effect closer to the cause, and the reader doesn't have to scan the rest of the code to make sure that the flag retains the value you would set.

Answer 4

Yes, it is a code smell (cue downvotes from everyone who does it).

The key thing for me is the use of the break statement. If you didn't use it then you would be iterating over more items than required, but using it gives two possible exit points from the loop.

Not a major issue with your example, but you can imagine that as the conditional or conditionals inside the loop become more complex or the ordering of the initial list becomes important then it's easier for a bug to creep into the code.

When the code is as simple as your example, it can be reduced to a while loop or equivalent map, filter construct.

When the code is complex enough to require flags and breaks it will be prone to bugs.

So as with all code smells: If you see a flag, try to replace it with a while. If you can't, add extra unit tests.

Answer 5

Just use a name other than limitFlag that tells what you are actually checking. And why do you log anything when the map is absent or empty? limtFlag will be false, all you care about. The loop is just fine if the map is empty, so no need to check that.

Answer 6

Setting a boolean value to convey information you already had is bad practice in my opinion. If there is no easy alternative then it's probably indicative of a bigger issue like poor encapsulation.

You should move the for loop logic into the fillUpList method to have it break if the limit is reached. Then check the size of the list directly afterwards.

If that breaks your code, why?

Answer 7

First off the general case: Using a flag to check if some element of a collection meets a certain condition is not uncommon. But the pattern I've seen most often to solve this is to move the check in an extra method and directly return from it (like Kilian Foth described in his answer):

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

Since Java 8 there is a more concise way using Stream.anyMatch(…):

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

In your case this would probably look like this (assuming list == entry.getValue() in your question):

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

---

The problem in your specific example is the additional call to fillUpList(). The answer depends a lot on what this method is supposed to do.

Side note: As it stands, the call to fillUpList() does not make much sense, because it does not depend on the element you are currently iterating. I guess this is a consequence of stripping down your actual code to fit the question format. But exactly that leads to an artificial example that is difficult to interpret and thus hard to reason about. Therefore it is so important to provide a Minimal, Complete, and Verifiable example.

So I assume that the actual code passes the current entry to the method.

But there are more questions to ask:

• Are the lists in the map empty before reaching this code? If so, why is there already a map and not just the list or set of the BigInteger keys? If they are not empty, why do you need to fill up the lists? When there are already elements in the list, isn't it an update or some other computation in this case?
• What causes a list to become larger than the limit? Is this an error condition or expected to happen often? Is it caused by invalid input?
• Do you need the lists computed up to the point you reach a list larger than the limit?
• What does the "Do something" part do?
• Do you restart the filling after this part?

This are only some questions that came to my mind when I tried to understand the code fragment. So, in my opinion, that is the real code smell: Your code does not clearly communicate the intent.

It could either mean this ("all or nothing" and reaching the limit indicates an error):

/**
 * 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.
}

Or it could mean this ("update until first problem"):

/**
 * 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.
}

Or this ("update all lists but keep original list if it becomes too large"):

/**
 * 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
}

Or even the following (using computeFoos(…) from the first example but without exceptions):

/**
 * 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);
}

Or it could mean something entirely different… ;-)