-1

I have a loop that I am testing a condition in but if the condition is not met then after the loop is complete, I want to execute another code block:

Boolean loopConditionNotMetFlag = true;
for (List<String> element : myList) {
  if (isSomeCondition(element)) {
    // do something
    loopConditionNotMetFlag = false;
    break;
  }
}
        
if (loopConditionNotMetFlag) {
  // Do something else
}

Is there better way to do this without using the loopConditionNotMetFlag flag or is this a valid use of a flag variable and not a code smell?

9
  • 3
    This is a valid use for booleans.
    – Ccm
    Commented May 21 at 20:40
  • 1
    Would it make more sense to extract the loop into another function? So then this could be rewritten as string element = FindValidElement(myList); if (element != null) { /* do something */ } else { /* do something else */ }
    – Andrew
    Commented May 21 at 20:45
  • 1
    @Devlinite If that's the case, are you sure you want that break in your example?
    – JimmyJames
    Commented May 21 at 21:27
  • 7
    Unrelated to the core question: try avoiding negative naming (i.e. the "not" in loopConditionNotMetFlag). It will enhance readability to remove that negation, i.e. start off with loopConditionWasMet = false which you then eventually set to true. It works the same but requires less mental effort to read.
    – Flater
    Commented May 21 at 23:59
  • 1
    ... and I willl be happy to revoke my downvote after the question was improved.
    – Doc Brown
    Commented May 22 at 11:25

4 Answers 4

4

Python has a special for ... else syntax just for that control flow pattern. So I'd say doing it like this in a language that doesn't have special support is probably as good as it gets.

2
  • Probably forty years ago I found a suggestion for an "n + 1/2" loop, may have been Knuth: for (normal loop until cond1 or cond2 or cond3) { ... if (condition) break cond1; } case cond1: "Handle break cond1;" case cond2: "Handle break cond2; " default: "Handle no break; ". So you run a loop until you exit because one of several conditions are met, and immediately following the loop code handling each condition, similar to a switch statement.
    – gnasher729
    Commented May 22 at 11:12
  • 1
    for ... else is the most wonderful construct that probably shouldn't be used. That is, it's a really good idea with a lot of utility but it's also very unusual and poorly understood by most developers. It's a shame, really.
    – JimmyJames
    Commented May 22 at 15:03
3

I don't think it's a code smell. It's just that your intentions would be clearer with the stream API. It's more readable like this because it says what you do, you just have to read the code:

myList                                // You have a list,
  .stream()                           // and you walk through it,
  .filter(this::isSomeCondition)      // but you have a condition
  .findFirst()                        // and you need the first element matching it.
  .ifPresentOrElse(
    firstElement -> {                 // With that element,
      // Do something                 // you do something.
    },
    () -> {                           // But if that element is not found,
      // Do something else            // you do something else.
    });
2
  • IMHO it is a code smell when it is not easily possible to refactor the code this way. Since the OP wrote "I'm more building up a list in the loop than finding a specific element." in a comment (which they "forgot" to tell us in the question), I think this is probably the case here.
    – Doc Brown
    Commented May 22 at 11:22
  • Yes I've seen that comment after I wrote my answer. I admit I don't really understand what the OP means by that... but I think my answer is valid for the question.
    – Mathieu
    Commented May 23 at 6:38
0

If you break only on loopConditionNotMetFlag, it might be possible to use a return. This idiom often used when finding the first something in a list. (As noted in the @Andrew comment) e.g.

for (List<String> element : myList) {
  if (isSomeCondition(element)) {
    return element;  // found it
  }
}
        
// at this point, we know that loopConditionNotMetFlag
{
  // return null, or Do something else
}

Added: I see from a comment by OP that this approach may be unsuitable for their needs. I'll leave this answer up a little while just in case...

2
  • 1
    Yes, it might be possible. But why would you do that? The code is self-explanatory to anyone other than the newest of newbies. And a good compiler will optimise the variable away anyway.
    – gnasher729
    Commented May 22 at 0:49
  • If you only want to find the first element in a list, there may be a function "first" for example doing that for you, so there is no reason writing another function yourself.
    – gnasher729
    Commented May 22 at 11:16
0

The "loop" and "Flag" in "loopConditionNotMetFlag" give no information whatsover and are just noise. Better: "conditionNotMet" or just "notFound".

If available, you can look for more powerful APIs that make this easier. For example in Swift (since apparently you want to handle the first item meeting the condition only):

if let first = myList.first { elem in elem.someCondition() } {
    // Do something with first
} else {
    // Do something else
}

or if not finding an element is more an error that should be guarded against:

guard let first = myList.first { $0.someCondition() } else {
    // Do something else
    return
}
// Do something with first

}

Both ways may actually be faster than doing this "by hand", since "first" is a highly optimised function, likely completely inlined, and possibly taking shortcuts while reference counting. Also works for everything supporting the "Sequence" protocol without any source code change.

2
  • OP's comments say that he is building up a list, not finding the first.
    – user949300
    Commented May 22 at 17:11
  • His code is finding the first.
    – gnasher729
    Commented May 22 at 18:00

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