30

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?

  • 10
    Why do you feel it's a code smell? what kind of specific problems can you foresee when doing this which wouldn't happen under a different structure? – Ben Cottrell Jan 22 '18 at 7:31
  • 13
    @gnasher729 Just out of curiousity, which term would you use instead? – Ben Cottrell Jan 22 '18 at 7:35
  • 11
    -1, your example makes no sense. entry is nowhere used inside the function loop, and we can only guess what list is. Is fillUpList supposed to fill list? Why doesn't it get it as a parameter? – Doc Brown Jan 22 '18 at 7:48
  • 13
    I'd reconsider your use of whitespace and empty lines. – Daniel Jour Jan 22 '18 at 11:55
  • 11
    There's no such thing as code smells. "Code smell" is a term invented by software developers who want to hold their nose when they see code that doesn't meet their elitist standards. – Robert Harvey Jan 22 '18 at 18:03
70

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()) {
  ...
  • 6
    Actually the smell in his code is the long function which needs to be split into smaller functions. Your suggestion is the way to go. – Bernhard Hiller Jan 22 '18 at 9:05
  • 2
    A better phrase that describes the useful function of the first part of that code is finding whether the limit will be exceeded after it accumulates something from those mapped items. We can also safely assume that fillUpList() is some code (which OP decides not to share) that actually uses the value entry from the iteration; without this assumption, it would look like the loop body didn't use anything from the loop iteration. – rwong Jan 22 '18 at 10:11
  • 4
    @Kilian: I just have one concern. This method will fill up a list and will be returning a Boolean which represents that list size exceeds a limit or not, so the name 'fill_list_from_map' does not make it clear that what does the Boolean returned represent (it failed to fill up, a limit exceeds, etc). As the Boolean returned is for a special case which ain't obvious from function name. Any comments ? PS: we can take command query separation into consideration too. – Siddharth Trikha Jan 22 '18 at 12:18
  • 2
    @SiddharthTrikha You are right, and I had the exact same concern when I suggested that line. But it's unclear which list the code is supposed to fill. If it's always the same list, you don't need the flag, you can simply check its length afterward. If you do need to know whether any individual fill-up exceeded the limit, then you do have to transport that information outside somehow, and IMO the command/query separation principle isn't a sufficient reason to reject the obvious way: via the return value. – Kilian Foth Jan 22 '18 at 13:06
  • 6
    Uncle Bob says on page 45 of Clean Code: "Functions should either do something or answer something, but not both. Either your function should change the state of an object, or it should return some information about that object. Doing both often leads to confusion." – CJ Dennis Jan 23 '18 at 0:57
25

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.

  • 2
    This was the reason I felt that it's a code smell, as the the blocks are not completely isolated and can be hard to track later. So I guess code in @Kilian 's answer the closest we can get ? – Siddharth Trikha Jan 22 '18 at 8:20
  • 1
    @SiddharthTrikha: It is hard to say since I don't know what the code is actually supposed to do. If you only want to check if the map contains at least one item which a list larger than limit, I think you can do it with a single anyMatch expression. – JacquesB Jan 22 '18 at 8:38
  • 2
    @SiddharthTrikha: the scope problem can easily be solved by changing the initial test to a guard clause like if(map==null || map.isEmpty()) { logger.info(); return;} This will, however, work only if the code we see is the full body of a function, and the // Do something part is not required in case the map is null or empty. – Doc Brown Jan 22 '18 at 10:52
14

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.

5

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.

  • +1 from me. It's a code smell for sure and you articulate why, and how to handle it, well. – David Arno Jan 22 '18 at 11:01
  • @Ewan: SO as with all code smells: If you see a flag, try to replace it with a while Can you elaborate on this with an example ? – Siddharth Trikha Jan 22 '18 at 11:20
  • 2
    Having multiple exit points from the loop may make it harder to reason about, but in this case so would refactoring it to make the loop condition depend on the flag - it'd mean replacing for (Map.Entry<BigInteger, List<String>> entry : map.entrySet()) with for (Iterator<Map.Entry<BigInteger, List<String>>> iterator = map.entrySet().iterator(); iterator.hasNext() && !limitFlag; Map.Entry<BigInteger, List<String>> entry = iterator.next()). That's an uncommon enough pattern that I'd have more trouble understanding it than a relatively simple break. – James_pic Jan 22 '18 at 11:49
  • @James_pic my java is a bit rusty, but if we are using maps then I would I would use a collector to sum up the number of items and filter out those after the limit. However, as I say the example is "not that bad" a code smell is a general rule that warns you of a potential problem. Not a sacred law you must always obey – Ewan Jan 22 '18 at 12:06
  • 1
    Don't you mean "cue" rather than "queue"? – psmears Jan 22 '18 at 13:00
0

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.

0

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?

0

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… ;-)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.