48

Sometimes I need for loops which needs a break like this:

for(int i=0;i<array.length;i++){
    //some other code
    if(condition){
        break;
    }
}

I feel uncomfortable with writing

if(condition){
    break;
}

because it consumes 3 lines of code. And I found the loop can be rewritten as:

                                   ↓
for(int i=0;i<array.length && !condition;i++){
    //some other code
}

So my question is, is it good practice to move the condition into the condition field to reduce lines of codes if possible?

closed as too broad by gnat, amon, Thomas Owens Mar 3 '18 at 23:41

Please edit the question to limit it to a specific problem with enough detail to identify an adequate answer. Avoid asking multiple distinct questions at once. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 7
    Depends on what the condition in particular is. – Bergi Feb 27 '18 at 13:44
  • 4
    There is a reason why misra forbid use of "break" and "continue" within loops. See also this: stackoverflow.com/q/3922599/476681 – BЈовић Feb 27 '18 at 14:26
  • 35
    I think the number of lines itself is not the real issue. It could be written on one line. if(condition) break; The issue in my opinion is readability. I personally dislike breaking a loop other than using the loop condition. – Joe Feb 27 '18 at 15:59
  • 97
    because it consumes 3 lines of code A very, very, very bad reason to dislike any style. IMO "consum[ing] lines of code" is somewhere between irrelevant and a good thing. Trying to stuff too much logic into too few lines results in unreadable code and hard-to-find bugs. – Andrew Henle Feb 27 '18 at 16:40
  • 3
    Possibly unpopular opinion: for(int i=0;i<array.length && !condition;i++) is relatively uncommon and may be overlooked by someone just skimming the code; this might be a good use case for a while or do while loop, which more often have multiple break conditions in the loop definition. – Jules Feb 28 '18 at 22:57

16 Answers 16

155

Those two examples you gave are not functionally equivalent. In the original, the condition test is done after the "some other code" section, whereas in the modified version, it is done first, at the start of the loop body.

Code should never be rewritten with the sole purpose of reducing number of lines. Obviously it's a nice bonus when it works out that way, but it should never be done at the expense of readability or correctness.

  • 5
    I understand your sentiment, but I've maintained enough legacy code where the loop is several hundred lines long and there are multiple conditional breaks. It makes debugging very difficult when you expect code to get to the conditional break you expect but prior code exercised it's conditional break first. How do you handle that code? In short examples like the above, it's easy to forget about that kind of all too common scenario. – Berin Loritsch Feb 27 '18 at 13:49
  • 84
    @BerinLoritsch: The correct way to deal with that is to not have loops that are several hundred lines long! – Chris Feb 27 '18 at 13:58
  • 27
    @Walfrat: If rewriting it isn't an option then there isn't really a question any more. Having said that you can usually pretty safely use an "Extract Method" type refactor to move blocks of code around which should make your main method loop shorter and hopefully make the logic flow more obvious. Its really a case by case thing. I'll stand by my general rule of keeping methods short and definitely keep nexted loop logic short but I don't want to try to say anything more than that in a general sense though... – Chris Feb 27 '18 at 14:35
  • 4
    @AndrewHenle brevity is readability, at least in the sense that increasing verbosity always reduces readability and maintainability. Reducing LOC is achieved by increasing density and raising the abstraction level, and is very tightly correlated with maintainability as attested in other Q/A pairs on this very site. – Leushenko Feb 28 '18 at 9:08
  • 5
    @Joe a Do-While construct is so rare that it is prone to trip developers that need to maintain that code later on. A few devs never actually saw them! I'm not sure if a do-while would improve readability at all - if anything, it would increase the difficulty to understand the code. Just as an example - my current codebase is around 15 years old and has around 2 million LOC. Around fifty developers touched it already. It has exactly zero do-while loops! – T. Sar Feb 28 '18 at 12:18
51

I don't buy the argument that "it consumes 3 lines of code" and thus is bad. After all, you could just write it as:

if (condition) break;

and just consume one line.

If the if appears half way through the loop, it of course has to exist as a separate test. However, if this test appears at the end of the block, you have created a loop that has a continue test at both ends of the loop, possibly adding to the code's complexity. Be aware though, that sometimes the if (condition) test may only make sense after the block has executed.

But assuming that's not the case, by adopting the other approach:

for(int i=0;i<array.length && !condition;i++){
    //some other code
}

the exit conditions are kept together, which can simplify things (especially if "some other code" is long).

There's no right answer here of course. So be aware of the idioms of the language, what your team's coding conventions are etc. But on balance, I'd adopt the single test approach when it makes functional sense to do so.

  • 14
    @jpmc26, Better? No. Valid alternative style? Of course. – David Arno Feb 27 '18 at 16:48
  • 6
    Better as in harder to mess up when the code is edited later, and not suffering from any notable disadvantages. – jpmc26 Feb 27 '18 at 16:50
  • 4
    Keeping them together because of "loop contents might be long" seems to be a weak argument. When your for content is hard to read then you have other problems than saving the two lines of code. – BlueWizard Feb 27 '18 at 18:37
  • 12
    @jpmc26 Disagree. Braces are superfluous clutter in the shortest, one-liner conditionals as per this answer. Without is more elegant and easier on the eye. They're like a ternary conditional operator. I have never forgotten to add the braces while breaking them out into bigger statement blocks; I'm already adding newlines after all. – benxyzzy Feb 27 '18 at 19:35
  • 3
    It's all a style preference, but if one makes the argument that it's safer, I would disagree with that reasoning. I myself prefer without braces and on the next line, but that doesn't mean it's any more or less valid than other styles – phflack Feb 27 '18 at 19:59
49

This sort of question has sparked debate almost as long as programming as been going. To throw my hat into the ring, I'd go for the version with the break condition and here's why (for this specific case):

The for loop is just there to specify the iterating values in the loop. Coding the breaking condition within that just muddies the logic IMO.

Having a separate break makes it crystal clear that during normal processing, the values are as specified in the for loop and processing should continue except where the condition comes in.

As a bit of background, I started off coding a break, then put the condition in the for loop for years (under the direction of an exceptional programmer) and then went back to the break style because it just felt far cleaner (and was the prevailing style in the various code tomes I was consuming).

It is another of those holy wars at the end of the day - some say you should never break out of a loop artificially, otherwise it isn't a real loop. Do whatever you feel is readable (both for yourself and others). If reducing keystrokes is really your thing, go code golfing at the weekend - beer optional.

  • 3
    If the condition has a good name, such as "found" (e.g. you are looking through the array for something), then it is a fine idea to put it in the head of the for loop. – user949300 Feb 27 '18 at 20:52
  • 1
    I was told that for loops are used when the number of iterations are known (e.g when there's no breaking condition but the end of the array/list) and while when are not. This seems to be the case for a while loop. If readability is the concern a idx+=1 inside the loop is much cleaner to read than a if/else block – Laiv Feb 28 '18 at 6:34
  • You can't just put the break condition in the loop if there is code following it, so at least you would have to write "if (condition) { found = true; continue; } – gnasher729 Feb 28 '18 at 10:07
  • We are so used to for(int i=0; i<something;i++) loops that I guess half the developers don't really remember that for loops can be used differently, and thus have a hard time understanding the &&condition part. – Ralf Kleberhoff Mar 1 '18 at 8:51
  • @RalfKleberhoff Indeed. I've seen many a new developer express surprise that the tripart statement expressions are all optional. I don't even recall the last time I saw for(;;)... – Robbie Dee Mar 1 '18 at 13:03
43

for loops are for iteration over something1 - they aren't just lol-let's-pull-some-random-stuff-from-the-body-and-put-them-in-the-loop-header - the three expressions have very specific meanings:

  • The first one is for initializing the iterator. It can be an index, a pointer, an iteration object or whatever - as long as it is used for iteration.
  • The second is for checking if we reached the end.
  • The third is for advancing the iterator.

The idea is to separate the iteration handling (how to iterate) from the logic inside the loop (what to do in each iteration). Many languages usually have a for-each loop that relieves you from the details of the iteration, but even if your language doesn't have that construct, or if it can't be used in your scenario - you should still limit the loop header to iteration handling.

So, you need to ask yourself - is your condition about the iteration handling or about the logic inside the loop? Chances are, it's about the logic inside the loop - so it should be checked inside the loop rather than in the header.

1As opposed to other loops, that are "waiting" for something to happen. A for/foreach loop should have the concept of a "list" of items to iterate on - even if that list is lazy or infinite or abstract.

  • 5
    This was my first thought when I read the question. Disappointed I had to scroll through half a dozen answers to see someone say it – Nemo Feb 27 '18 at 17:46
  • 4
    Umm... all loops are for iteration - that's what the word "iteration" means :-). I assume you mean something like "iterating over a collection", or "iterating using an iterator"? – psmears Feb 27 '18 at 17:57
  • 3
    @psmears OK, maybe I chose the wrong word - English is not my native tongue, and when I think of iteration I think of "iteration over something". I'll fix the answer. – Idan Arye Feb 27 '18 at 20:15
  • An intermediate case is where the condition is something like someFunction(array[i]). Now both parts of the condition are about the thing you're iterating over, and it would make sense to combine them with && in the loop header. – Barmar Feb 28 '18 at 23:01
  • I respect your position, but I disagree. I think for loops at heart are counters, not loops over a list, and this is supported by the syntax of for in earlier languages (these languages often had a for i = 1 to 10 step 2 syntax, and the step command - even allowing an initial value that is not the index of the first element - hints at a numerical context, not a list context). The only use case that I think supports for loops as only iterating over a list is parallelising, and that requires explicit syntax now anyway in order to not break code doing, for example, a sort. – Logan Pickup Mar 1 '18 at 14:54
8

I recommend using the version with if (condition). It's more readable and easier to debug. Writing 3 extra lines of code won't break your fingers, but will make it easier for the next person to understand your code.

  • 1
    Only if (like has been commented) the loop block doesn't has hundreds lines of code and the logic within is not rocket science. I think your argument is ok, but I miss something like naming things properly in order to understand what's going on and when the breaking condition is met. – Laiv Feb 28 '18 at 6:47
  • 2
    @Laiv If the loop block has hundreds lines of code within it, then the are bigger problems than the question where to write the break condition. – Aleksander Feb 28 '18 at 9:33
  • That's why I suggested contextualise the answer, because is not always true. – Laiv Feb 28 '18 at 9:36
8

If you are iterating over a collection where certain elements should be skipped, then I recommend a new option:

  • Filter your collection before iterating

Both Java and C# make this relatively trivial to do. I wouldn't be surprised if C++ had a way of making a fancy iterator to skip certain elements that don't apply. But this is only really an option if your language of choice supports it, and the reason your are using break is because there are conditions on whether you process an element or not.

Otherwise there is a very good reason to make your condition a part of the for loop--assuming it's initial evaluation is correct.

  • It's easier to see when a loop ends early

I've worked on code where your for loop takes a few hundred lines with several conditionals and some complex math going on in there. The problems you run into with this are:

  • break commands burried in the logic are hard to find and sometimes surprising
  • Debugging requires stepping through each iteration of the loop to truly understand what's going on
  • A lot of the code does not have easily reversable refactorings available or unit tests to help with functional equivalence after a rewrite

In that situation I recommend the following guidelines in order of preference:

  • Filter the collection to eliminate the need for a break if possible
  • Make the conditional part of the for loop conditions
  • Place the conditionals with the break at the top of the loop if at all possible
  • Add a multi-line comment drawing attention to the break, and why it is there

These guidelines are always subject to the correctness of your code. Choose the option that can best represent the intent and improve the clarity of your code.

  • 1
    Most of this answer is fine. But... I can see how filtering a collection could eliminate the need for continue statements, but I don't see how they help much with break. – user949300 Feb 27 '18 at 20:55
  • 1
    @user949300 A takeWhile filter can replace break statements. If you have one - Java for example only gets them at Jave 9 (not that it's that hard to implement it yourself) – Idan Arye Feb 28 '18 at 1:35
  • 1
    But in Java you still can filter before the iteration. Using streams (1.8 or later) or using Apache Collections (1.7 or earlier). – Laiv Feb 28 '18 at 6:44
  • @IdanArye - there are add-on libraries that add such facilities to the Java streams API (e.g. JOO-lambda) – Jules Feb 28 '18 at 17:41
  • @IdanArye never heard of a takeWhile before your comment. Explicitly linking the filter to an ordering strikes me as unusual and hacky, since in a "normal" filter every element gets to return true or false, independent of what comes before or after. In this way you can run things in parallel. – user949300 Mar 2 '18 at 19:55
8

I strongly recommend to take the approach of least surprise unless you gain a significant benefit doing otherwise.

People don't read every letter when reading a word, and they don't read every word when reading a book - if they're adept at reading, they look at the outlines of words and sentences and let their brain fill in the rest.

So chances are the occasional developer will assume this is just a standard for loop and not even look at it:

for(int i=0;i<array.length&&!condition;i++)

If you want to use that style regardless, I recommend changing the parts for(int i=0;i< and ;i++) that tell the reader's brain that this is a standard for loop.


Another reason to go with if-break is that you cannot always use your approach with the for-condition. You have to use if-break when the break condition is too complex to hide within a for statement, or relies on variables that are only accessible inside the for loop.

for(int i=0;i<array.length&&!((someWidth.X==17 && y < someWidth.x/2) || (y == someWidth.x/2 && someWidth.x == 18);i++)

So if you decide to go with if-break, you're covered. But if you decide to go with for-conditions, you have to use a mix of if-break and for-conditions. To be consistent, you will have to move the conditions back and forth between the for-conditions and the if-break whenever the conditions change.

4

Your transformation is assuming that whatever condition is evaluates to true as you enter the loop. We can't comment on the correctness of that in this instance because it isn't defined.

Having succeed in "reducing lines of code" here, you may then go on to look at

for(int i=0;i<array.length;i++){
    // some other code
    if(condition){
        break;
    }
    // further code
}

and apply the same transformation, arriving at

for(int i=0;i<array.length && !condition;i++){
    // some other code
    // further code
}

Which is an invalid transformation, as you now unconditionally do further code where you previously didn't.

A much safer transformation scheme is to extract some other code and evaluating condition to a separate function

bool evaluate_condition(int i) // This is an horrible name, but I have no context to improve it
{
     // some other code
     return condition;
}

for(int i=0;i<array.length && !evaluate_condition(i);i++){
    // further code
}
4

TL;DR Do whatever reads best in your particular circumstance

Let's take this example:

for ( int i = 1; i < array.length; i++ ) 
{
    if(something) break;
    // perform operations
}

In this case we don't want any of the code in the for loop to execute if something is true, so moving the test of something into the condition field is reasonable.

for ( int i = 1; i < array.length && !something; i++ )

Then again, depending on if something can be set to true before the loop, but not within it, this might could offer more clarity:

if(!something)
{
    for ( int i = 1; i < array.length; i++ )
    {...}
}

Now imagine this:

for ( int i = 1; i < array.length; i++ ) 
{
    // perform operations
    if(something) break;
    // perform more operations
}

We're sending a very clear message there. If something becomes true while processing the array then abandon the whole operation at this point. In order to move the check into the condition field you need to do:

for ( int i = 1; i < array.length && !something; i++ ) 
{
    // perform operations
    if(!something)
    {
        // perform more operations
    }
}

Arguably, the "message" has become muddied, and we are checking the failure condition in two places - what if the failure condition changes and we forget one of those places?

There are of course many more different shapes a for loop and condition could be, all with their own nuances.

Write the code so that it reads well (this is obviously somewhat subjective) and concisely. Outside of a draconian set of coding guidelines all "rules" have exceptions. Write what you feel expresses the decision making best, and minimize the chances for future mistakes.

2

While it may be appealing because you see all conditions in the loop header and break may be confusing inside large blocks, a for loop is a pattern where most programmers expect looping over some range.

Especially since you do so, adding an additional break condition can cause confusion and it is harder to see if it is functionally equivalent.

Often a good alternative is to use a while or do {} while loop which checks both conditions, assuming your array has at least one element.

int i=0
do {
    // some other code
    i++; 
} while(i < array.length && condition);

Using a loop which is only checking a condition and not running code from the loop header you make very clear when the condition is checked and what's the actual condition and what's code with side effects.

  • Despite this question having some good answers already, I wanted to specifically upvote this because it makes an important point: to me, having anything other than a simple bound checking in the for-loop (for(...; i <= n; ...)) actually makes the code harder to read because I have to stop and think about what is happening here and might miss the condition altogether on the first pass because I don't expect it to be there. For me, a for loop implies that you are looping over some range, if there is a special exit condition it should be made explicit with an if, or you can use a while. – CompuChip Feb 28 '18 at 9:09
1

This is bad, as code is meant to be read by humans - non-idiomatic for loops are often confusing to read, which means the bugs are more likely to be hiding here, but the original code may have been possible to improve while keeping it both short and readable.

If what you want is to find a value in an array (the provided code sample is somewhat generic, but it may as well be this pattern), you can just explicitly try to use a function provided by a programming language specifically for that purpose. For example (pseudo-code, as a programming language was not specified, the solutions vary depending on a particular language).

array.find(item -> item.valid)

This should noticeably shorten the solution while simplifying it as your code says specifically what you need.

1

Few reasons in my opinion that says, you should not.

  1. This reduces the readability.
  2. The output may not be same. However, it can be done with a do...while loop (not while) as the condition is checked after some code execution.

But on top of that, consider this,

for(int i=0;i<array.length;i++){

    // Some other code
    if(condition1){
        break;
    }

    // Some more code
    if(condition1){
        break;
    }

    // Some even more code
}

Now, can you really achieve it by adding these conditions into that for condition?

  • What is "openion"? Do you mean "opinion"? – Peter Mortensen Feb 28 '18 at 10:59
  • What do you mean by "Few reasons in openion that says, you should not"? Can you elaborate? – Peter Mortensen Feb 28 '18 at 11:00
0

I think removing the break can be a good idea, but not to save lines of code.

The advantage of writing it without break is that the post-condition of the loop is obvious and explicit. When you finish the loop and execute the next line of the program, your loop condition i < array.length && !condition must have been false. Therefore, either i was greater than or equal to your array length, or condition is true.

In the version with break, there’s a hidden gotcha. The loop condition says that the loop terminates when you’ve run some other code for each valid array index, but there is in fact a break statement that could terminate the loop before that, and you won’t see it unless you review the loop code very carefully. And automated static analyzers, including optimizing compilers, could have trouble deducing what the post-conditions of the loop are, too.

This was the original reason Edgar Dijkstra wrote “Goto Considered Harmful.” It wasn’t a matter of style: breaking out of a loop makes it hard to formally reason about the state of the program. I have written plenty of break; statements in my life, and once as an adult, I even wrote a goto (to break out of multiple levels of a performance-critical inner loop and then continue with another branch of the search tree). However, I am far more likely to write a conditional return statement from a loop (which never runs the code after the loop and therefore can’t muck up its post-conditions) than a break.

Postscript

In fact, refactoring the code to give the same behavior might actually need more lines of code. Your refactoring could be valid for particular some other code or if we can prove that condition will start off false. But your example is equivalent in the general case to:

if ( array.length > 0 ) {
  // Some other code.
}
for ( int i = 1; i < array.length && !condition; i++ ) {
  // Some other code.
}

If some other code isn’t a one-liner, you might then move it to a function so as not to repeat yourself, and then you’ve very nearly refactored your loop into a tail-recursive function.

I recognize this was not the main point of your question, though, and you were keeping it simple.

  • The problem is not that the break makes it hard to argue about the code, the problem is that break at random places makes the code to complicated things. If that is actually needed, then it's not hard to argue because of the break, but because the code needs to do complicated things. – gnasher729 Feb 28 '18 at 10:10
  • Here’s why I disagree: if you know there’s no break statement, then you know what the loop condition is and that the loop stops when that condition is false. If there’s a break? Then there are multiple ways the loop could terminate, and in the general case, figuring out when one of them could halt the loop is literally solving the Halting Problem. In practice, my bigger worry is that the loop looks like it does one thing, but actually, whoever wrote the code after the loop overlooked an edge case buried in its complex logic. Stuff in the loop condition is hard to miss. – Davislor Feb 28 '18 at 17:03
0

Yes, but your syntax is unconventional and hence bad(tm)

Hopefully your language supports something like

while(condition) //condition being a condition which if true you want to continue looping
{
    do thing; //your conditionally executed code goes here
    i++; //you can use a counter if you want to reference array items in order of index
}
  • 2
    maybe I should have used a different word for 'condition' I didn't mean it literally to mean the exact same condition in the example – Ewan Feb 27 '18 at 11:18
  • 1
    Not only is the for loop not even remotely unconventional, there is absolutely no reason a while loop of this form is better than an equivalent for loop. In fact most people would say it is worse, such as the authors of the CPP Core Guidelines – Sean Burton Feb 27 '18 at 17:34
  • 2
    Some people just hate creative alternatives. Or looking at a problem in a different way than the one set in their heads. Or more generally, smart asses. I feel your pain bro! – Martin Maat Feb 27 '18 at 18:33
  • 1
    @sean sorry dude, I know people hate to be told they are wrong, but I refer you to es.77 – Ewan Feb 27 '18 at 22:35
  • 2
    This while emphasizes the monster in the code - the exception otherwise hidden in routine/mundane code. The business logic is front and center, the looping mechanics is subsumed. It avoids the "wait a minute..." reaction to the for loop alternative. This answer fixes the problem. absolutely up vote. – radarbob Mar 2 '18 at 15:28
0

If you're concerned about an overly complex for statement being difficult to read, that is definitely a valid concern. Wasting vertical space is also an issue (albeit a less critical one).

(Personally I dislike the rule that if statements must always have braces, which is largely the cause of the wasted vertical space here. Note that the problem is wasting vertical space. Using vertical space with meaningful lines should not be a problem.)

One option is to split it to multiple lines. This is definitely what you should do if you're using really complex for statements, with multiple variables and conditions. (This is to me a better use of vertical space, giving as many lines as possible something meaningful.)

for (
   int i = 0, j = 0;
   i < array.length && !condition;
   i++, j++
) {
   // stuff
}

A better approach might be to try to use a more declarative and less imperative form. This will vary depending on the language, or you might need to write your own functions to enable this sort of thing. It also depends what condition actually is. Presumably it must be able to change somehow, depending on what is in the array.

array
.takeWhile(condition)  // condition can be item => bool or (item, index) => bool
.forEach(doSomething); // doSomething can be item => void or (item, index) => void
  • Splitting a complex or unusual for statement into multiple lines is the best idea in this whole discussion – user949300 Mar 2 '18 at 19:59
0

One thing that was forgotten: When I break from a loop (not do all the possible iterations, no matter how implemented), it's usually the case that not only do I want to exit the loop, I will also want to know why this happened. For example, if I look for an item X, not only do I break from the loop, I also want to know that X was found and where it is.

In that situation, having a condition outside the loop is quite natural. So I start with

bool found = false;
size_t foundIndex;

and in the loop I will write

if (condition) {
    found = true;
    foundIndex = i;
    break;
}

with the for loop

for (i = 0; i < something && ! found; ++i)

Now checking the condition in the loop is quite natural. Whether there is a "break" statement depends on whether there is further processing in the loop that you want to avoid. If some processing is needed and other processing is not, you might have something like

if (! found) { ... }

somewhere in your loop.

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