Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

While coding today, I came across the following loop in C#:

for(int x = 0; x < max_x_index; x++){
    for(int y = 0; y < max_y_index; y++, count++){
        some2DarrayRepresentedBy1D[count] = ...;
    }
}

Would this be consider bad practice? Would the following be a better way to achieve the same result:

for(int x = 0; x < max_x_index; x++){
    for(int y = 0; y < max_y_index; y++){
        some2DarrayRepresentedBy1D[count] = ...;
        count++;
    }
}

Would you allow both through, for example, a code review or would you tag the first example for being obscure?

share|improve this question
1  
I see this question has been tagged with both "C" and "Java". Idiomatic code in the two languages is different, so the best answer may well be different in different languages - for example, rolfl's some2Darray[count++] is common in C, but I don't see it very often in Java. –  Izkata Dec 8 '13 at 17:43
    
Ah, I see it wasn't you, but @200_success that added the tags... –  Izkata Dec 8 '13 at 17:47
1  
@Izkata some2Darray[count++] is just as valid in Java as in C. There's no reason or evidence to suggest that it's less prevalent in Java. –  200_success Dec 8 '13 at 18:05
2  
@200_success I never said anything about validity, I know it's valid in both languages. But idiomatic usage isn't always cross-language. –  Izkata Dec 8 '13 at 18:57
2  
Duh, of course it would. Thank you, @afuzzyllama. –  neontapir Dec 12 '13 at 21:31
show 3 more comments

7 Answers

up vote 26 down vote accepted

I would agree with the OP in that the second code block would be the best approach.

Reason - readability. In my opinion, the variable incremented in the for loop statement should only be used for keeping count of how many times we're looping. count might be set before the loop, it might also be incremented / decremented in several different loops. But in either case, count isn't used to keep track of the progress of the current loop - y is in this case.

So, I would say that it's best practice, for readability sake, to only increment the variable used to loop through the for loop in the for loop line itself.

share|improve this answer
add comment

Of all the answers so far, I think that Charlie74 is closest to reasoning the same way I do.

The for loop consists of three segments:

for (initialization; termination; increment) {
    statement(s)
}

it is my opinion that only things that affext the termination should be part of the increment.... In other words, if the value is not part of the termination condition then it should not be part of the increment.

In this case, the count does not affect the termination condition, and as a result it does not belong in the increment section..

For what it's worth, there are a couple of things I would do differently still:

  1. you have a typos in your second example.... (missing x in x < and missing y <) [This has been fixed in the question now.]
  2. I would include the count++ as part of the array assignment (which is where I think it belongs):
for(int x = 0; x < max_x_index; x++){
    for(int y = 0; y < max_y_index; y++){ 
        some2Darray[count++] = ...;
    }
}

Discussion on ++count vs. count++ in this context

is this one of those instances where if you change i++ to ++i it will change what is assigned in the array? could you explain that a little more ... ?

Here goes.... Yes, this is one of the situations where the the pre-increment or post-increment difference is significant. Wikipedia has a section on this.

The pre-increment operator ++count increments the count variable. The post-increment operator count++ also increments the count variable. The difference is that while both ++count and count++ are expressions, the resulting values of the expressions are different.

It is important to understand what it means that count++ is an expression. An expression is something that produces a value. In the context array[count] = 1, the count is an expression, it is something with a value. That expression could instead be a constant (array[0]), a function (array[getNextIndex()], or even a more complex expression consisting of sub-expressions and operators (array[5 - 3]).

So, count++ is an expression. It is also something that modifies the value in the count variable. Similarly, ++count is an expression, and it also modifies the value of count. The difference between them is not what they do to count, but it is the value of the expression.

With count++ the value of the expression is the value of count before it is incremented.

With ++count the value of the expression is the value of count after it is incremented.

Thus, in the example above, compare two different situations:

int count = 0;
array[count++] = 1;

vs.

int count = 0;
array[++count] = 1;

In the first example, the value of the expression count++ is 0 (the value before the increment) and as a result, the value at index 0 of the array is set to 1. I.e. it is equivalent to:

int count = 0;
array[count] = 1;
count = count +1;

in the second example, the value of the expression ++count is 1 (the value after the increment) and as a result, the value at index 1 of the array is set to 1. I.e. it is equivalent to:

int count = 0;
count = count + 1;
array[count] = 1;

It is obvious, that by using ++count we will start adding data at index 1 instead of 0, and we run the risk of an out-of-bounds operation when we get to the end of the data.

In the case of adding data to an array (like what this question is doing), it makes sense to set a counter to the 'next' slot to use, and then do a post-increment so that you 'use' the next slot, and then increment it so it is pointing to the next slot again.

A bonus of doing things this way is that, at the end, the count is also the number of slots in the array that you have used.

Serendipitously, another question came up ( Finding all indices of largest value in an array ) where one potential answer illustrates this point nicely.

share|improve this answer
1  
So you would put count++ into the increment section if the loop condition would contain for example && count < maxCount? –  Nobody Dec 8 '13 at 16:19
1  
Absolutely, yes. –  rolfl Dec 8 '13 at 16:27
    
@rolfl, is this one of those instances where if you change i++ to ++i it will change what is assigned in the array? could you explain that a little more, so someone doesn't get confused when they put [++count] –  Malachi Dec 10 '13 at 19:02
3  
@Malachi it will change where in the array it is assigned. For x=0;y=0 you will assign to position 0 with count++, and to position 1 with ++count. (Assuming count=0 before the loop.) –  ANeves Dec 10 '13 at 19:42
add comment

I think that @rolfl is on the right track with

for (int x = 0; x < max_x_index; x++) {
    for (int y = 0; y < max_y_index; y++) {
        some2Darray[count++] = ...;
    }
}

A good reason for using a for-loop rather than

/* initialization */;
while (/* condition */) {
    /* body */;

    /* increment */;
}

is to give your code an immediately recognizable appearance that matches its purpose. Therefore, the initialization, condition, and increment parts should all contribute to the same purpose. You should avoid introducing "off-topic" code in the for-loop header.

Additionally, some2Darray[count++] = ... effectively conveys, in just one line, that you want to populate the array — write an element, then move on to the next.


Other users here have objected that the count++ might be bypassed due to some unexpected flow control in the loop body. The goal of the code appears to be producing a flat 2D array, so I doubt that there would be any break or continue involved. However, if you are still concerned about consistency, you could use an assertion.

int count = 0;
for (int x = 0; x < max_x_index; x++) {
    for (int y = 0; y < max_y_index; y++) {
        some2Darray[count++] = ...;
    }
}
Debug.Assert(count == max_x_index * max_y_index);

Alternatively, eliminate count altogether and just use x * max_y_index + y.

share|improve this answer
    
As one of those pointing out that flow control can miss a trailing increment, I agree that in the given usage this isn't going to happen. But the point must be considered when giving general guidance about what to accept in a code review. –  Michael Urman Dec 8 '13 at 22:24
    
"Count" is more clear than "x * max_x_index + y", and it continues to work if the array becomes more complicated (for example if some iterations push zero elements or more than one element) –  Matt Dec 9 '13 at 1:54
add comment

There is a correctness concern with such a change. In the example given, the transformed loop is equivalent (aside from the order in which the increments occur; I'm assuming these are both int variables, so is unlikely to matter).

However if the body of the loop can contain the continue statement, suddenly there's a very large difference between these two:

int count = 0;
for (int x = 0; x < MAX; ++x, ++count)
{
    : : :
    if (sometest())
        continue;
    : : :
}
int count = 0;
for (int x = 0; x < MAX; ++x)
{
    : : :
    if (sometest())
        continue;
    : : :
    ++count;
}

In the latter code block, count is only incremented if the loop body completes. In the former code block, count is incremented whenever starting the next loop iteration. The use case may determine which of these is correct, and the decision between these may deserve a comment to help avoid breaking it later.

Note that moving ++count to the beginning of the second example would make it somewhat more similar to the first example, but in your example would require indexing your array with count - 1. As a similar alternative if you only use the index once, you could use a construct such as some2Darray[count++] instead of either, although like the original second code block this risks misbehavior from an early continue as well, and some would consider it less readable.

share|improve this answer
add comment

Several people on this thread have talked about how a for loop works, but few have mentioned the semantic difference between a for loop and a while loop.

When a programmer encounters a for loop when reading code, the semantic is that the loop definitely terminates and iterates over some range of numbers or some list of elements. That's why all programmers immediately know what the following loops do, and they are very easy to mentally understand and parse.

// iterate over the integer range N...M
for(int i = N; i < M; i++)
   func(N)

// iterate over every element in array
for(int i = 0; i < array.length; i++)
   func(array[i]);

// iterate over some singly linked list
for(item = list; item != NULL item=item->Next)
   func(item);

// iterate over some doubly linked list:
for(item = listHead->Next; item != listHead; item = item->Next)
   func(item);

For this reason, both of your first two arrays are well designed, and are quick and easy to understand during a code-review. The two lists are iterating x and y over all of the items [0,0] to [max_x_index, max_y_index].

But if you poison one of the two for loops so that they no longer look normal by adding some semantically unrelated variable (count) to one of the loops, suddenly the entire loop becomes harder to mentally understand. count is not related to [x,y] iterating over the loop. It's semantically related to how many elements are in some2DArray, and moving them far from each other makes the code as a whole harder to read, and inevitably, harder to maintain and debug as well.

Instead, good practice is to modify variables close to their semantic meaning, which aids comprehension of the code. To do this, you should modify count at the point where count ought to change - not when the loop iterates, but when you modify some2DArray either via inline increment:

 some2DArray[count++] = ...

or immediately afterwards:

 some2DArray[count] = ...
 count++

So in answer to your question, it's almost never a good idea to increment multiple variables within a for loop. The for loop should deal exclusively with modifying elements that are being iterated, and leave other variables well alone. Violation of this confuses the semantic of the program, and makes it much harder to read.

share|improve this answer
add comment

UPDATE

Matt made a really good point, but I still believe that the two following code blocks will sit well with what Matt said in his answer.

count is incremented every iteration of the inside for loop so it is changing where it should be changed in both blocks of code.

you should not increment a variable that is not declared in the Declaration of the For loop.

that said, you could do it like this

for(int x = 0, int count = 0; x < max_x_index; x++){
    for(int y = 0; y < max_y_index; y++, count++){ 
    {
        some2DarrayRepresentedBy1D[count] = ...;
    }
}

this is fairly confusing if you didn't write the code though, and thus violates the KISS Principle and/or the POLS/POLA Principle (Principle of Least Surprise/Astonishment)

going off the Answer given by Rolfl, this would be less code to write and probably slightly cleaner code, again, you should comment this code with what you did and why you do it this way.

for(int x = 0, int count = 0; x < max_x_index; x++){
    for(int y = 0; y < max_y_index; y++){ 
    {
        some2DarrayRepresentedBy1D[count++] = ...;
    }
}


Random bit of knowledge

what you could do with this

for(int x = 0; x < max_x_index; x++){
    for(int y = 0; y < max_y_index; y++){ 
    {
        some2DarrayRepresentedBy1D[count] = ...;
        count++;
    }
}

because it the incrementers are only used to progress the loops (depending on what you are counting, [this example will give you a completely different result])

for (int x = 0, int y = 100;x < max_x_index, y > max_y_index; x++, y--){
    some2DarrayRepresentedBy1D[count] = ... ;
    count++;
}

I only give this as an example that you can use two incrementers in one for loop, but what the OP posted is not one of those instances.

share|improve this answer
2  
I think rather than KISS you mean principle of least surprise? –  Nobody Dec 10 '13 at 18:57
    
@Nobody that is exactly what I was going to comment: "you mean POLS" :) –  Mat's Mug Dec 10 '13 at 19:06
    
it is the best way to increment that variable if you want to put it into the for loop and eliminate extra lines, maybe not extra comments or characters. I would love to see the stats on performance pitting the two ways against each other though –  Malachi Dec 10 '13 at 19:26
add comment

You mentioned in a comment that ultimately the end result will be setting every member of an array up to x * y

@neontapir - because y resets every iteration of x. count will end with a value of x * y. – afuzzyllama 1 hour ago

so here is my thought, Instead of doing this:

for(int x = 0; x < max_x_index; x++){
    for(int y = 0; y < max_y_index; y++, count++){ 
    {
        some2DarrayRepresentedBy1D[count] = ...;
    }
} 

Do this

for (int i = 0; i < max_x_index * max_y_index; i++)
{
    some2DarrayRepresentedBy1D[i] = ...;
}

obviously I don't know what you are doing in ... but if you are nested in two For loops and there isn't much else going on here then I assume that you are doing the same thing every time. this looks like the answer that you may be looking for, if not you need to show more code.

share|improve this answer
    
Presumably, the ellipsis in the original code was someProcess(someMatrix[x][y]). With this proposal, it would have to be someProcess(someMatrix[i / max_y_index][i % max_y_index]). I wouldn't consider that an improvement in either clarity or performance. –  200_success Dec 12 '13 at 23:16
    
@200_success, that formula doesn't seem right to me, but if that was the case then it would make sense to perform it like my other answer. but that information should be in the question, even though OP is asking something else entirely –  Malachi Dec 12 '13 at 23:21
    
@Malachi - the point of the question is to ask about the placement of count in the for declaration and if it was common or not –  afuzzyllama Dec 13 '13 at 0:56
    
@afuzzyllama I know, I read the question after words and realized I was very far from what you were asking. but I hope that it gives you another way to look at your code as well. from what 200_success commented though I don't think this specific answer will work. my other answer should work nicely for you and doesn't seem to break any rules –  Malachi Dec 13 '13 at 3:27
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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