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?

doppelgreener
  • 1,304
ocomfd
  • 5,750

16 Answers16

153

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.

Pete
  • 3,231
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.

David Arno
  • 39,599
  • 9
  • 94
  • 129
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.

Robbie Dee
  • 9,823
42

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.

Idan Arye
  • 12,145
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.

Robbie Dee
  • 9,823
Aleksander
  • 368
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.

Berin Loritsch
  • 46,426
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.

Peter
  • 3,778
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
}
Caleth
  • 12,190
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.

Grimm The Opiner
  • 294
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.

allo
  • 164
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?

Harsh Shankar
  • 119
  • 5
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.

null
  • 288
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
}
Ewan
  • 83,178
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.

Davislor
  • 1,563
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
Dave Cousineau
  • 355
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.

gnasher729
  • 49,096