Bad loop style

*pdest = source[si + i]+(((source[si + i + tracks] - source[si + i])*temp)>>FBITS);
*pdest  = source[si + i];
//dest[di+ i* trackStride] = 0xff;
//if(si+i+track> dwsamplenumber)
//  printf("the audio out range %d,%d,%d\n",si,i,track);

Why is this loop bad?
First, you can’t see what this loop is iterated for. It just looks very complicated.

Second, you can’t see which is which due to so many parenthesis in it.

Third, no spacing for operators makes it hard to read.

Then how about this?

pdest = dest + di;
for( i = 0; i < tracks; i++ )

    pdest = (SHORT *)( (UCHAR *)pdest + trackStride );


Now, it is very clear that it does something for each track. So, the loop is for each track.
Also, it is very easy to figure out what are the starting values of the i and pdest.
Plus, it is very clear to see how the pdest is changed.
Because there are spaces before and after operators, it is very pleasant to read.

When I worked for a Wilshire Associates Inc. there was a programmer who liked the bad style.
His logic was that he could see more information in a window.
Yeah.. it is true. However, what if it is very difficult to understand what they are?
Also, the for() line in the bad style case is very long and hard to read, while the good example is very short and more readable.

I would rather read lines of codes by understanding it than just looking at more codes.
It is also easier to make mistake when you edit the bad example case.


Leave a Reply

Please log in using one of these methods to post your comment: Logo

You are commenting using your account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )


Connecting to %s

%d bloggers like this: