current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote -3 down vote favorite
1

Which is better?

for (int Bw_1 = (2-Drctn)*(Drctn%2), Bw_2 = (Drctn-3)*((Drctn-1)%2), Cnt = 1;
     Cnt <= Rbn_Pxl; XFlush(Dsp), Cnt++, usleep(Usr_Draw_Arcs_Delay_Usec))

or 

for (int Bw_1 = (2 - Drctn) * (Drctn % 2), Bw_2 = (Drctn - 3) * ((Drctn - 1) % 2), Cnt = 1;
     Cnt <= Rbn_Pxl; XFlush(Dsp), Cnt++, usleep(Usr_Draw_Arcs_Delay_Usec))

or any good advice?

share|improve this question
8  
Next time, please provide some more context about what your code actually does, and provide a little more detailed code/question than just a difference in spacing. The lack of context is the reason why you are currently being down-voted. If you provide sufficient context, you will probably be up-voted a lot next time! –  Simon André Forsberg Jun 15 at 18:46
add comment

3 Answers

up vote 13 down vote accepted

Of those two, the second variant is better in my opinion because of the increased spacing.

However, both of them are horrible because of the amount of information pressed into those few lines.

I think your loop should be converted into a while instead.

int Bw_1 = (2 - Drctn) * (Drctn % 2);
int Bw_2 = (Drctn - 3) * ((Drctn - 1) % 2);
int Cnt = 1;
while (Cnt <= Rbn_Pxl) {
    ...
    XFlush(Dsp);
    Cnt++;
    usleep(Usr_Draw_Arcs_Delay_Usec);
}

Remember that all for-loops can be converted into a while because a for is separated into:

  1. Initialization.
  2. Condition. Loop as long as this is true.
  3. Statement to perform after each iteration.

As for the code itself:

You're using variable names that doesn't make sense at all. Except Cnt which is obviously short for count (what kind of count doesn't tell the story though). There's normally no need to save a few characters when naming variables, it wll oly mke tngs hrdr t rd. (Did I make the point clear?)

share|improve this answer
    
Clear. :-) Thanks for your opinion. –  Kevin Dong Nai Jia Jun 15 at 18:52
    
The trouble here is that without knowing what ... is this may not be equivalent to the original. You need to account for continue. Therefore I would leave the for(;;) loop in place; but move the initialization like you did and put the three statements you moved into the while loop into a separate function which I call from the last past of the for statement. for (int Cnt = 1; Cnt <= Rbn_Pxl; update(Cnt)) –  Loki Astari Jun 15 at 20:57
    
The problem with the code isn't related to the for loop as such. Generally, for loops are easier to read and sometimes also easier for the compiler to optimize. They are preferred when the number of iterations is known in advance. –  Lundin Jun 16 at 8:23
add comment
up vote 10 down vote

I agree with @Simon that…

  • the question is poorly posed, as we have no idea what the code intends to do
  • the variable names are needlessly cryptic
  • the for-loop header unreadable because it is doing way too much.

However, I would still use a for-loop. You just need to use it responsibly. The three parts of the loop header — initializer, condition, and advancement — should tell a coherent story. Everything else belongs outside the loop header.

int Bw_1 = …;
int Bw_2 = …;
for (int Cnt = 1; Cnt <= Rbn_Pxl; Cnt++) {
    …
}

Then you can immediately recognize how the loop uses Cnt.

share|improve this answer
add comment
up vote 6 down vote

Both versions are unreadable, because of the following problems:

  • The 3 expressions that make a for loop should be kept simple (1):

    • The first expression should only be concerned with the declaration/initialization of the loop iterator. There can only be one iterator, so there should only be one initialization here.
    • The second expression should only be concerned with the loop condition.
    • The third expression should only be concerned with the iteration. Keep it as simple as possible.

  • Whenever the comma operator is used in a C program, it is usually an indication of bad program design (2). Such code needs to be revised in most cases.

  • Consider using better variable names so that the code speaks for itself, without the need for external documentation. Instead of "Cnt", I assume you mean "Count". Instead of "Drctn", I assume you mean "Direction". By "Usr" I assume you mean "User". And so on. C allows verbose variable names.

  • In case this is a DSP program or any other embedded system, the use of int is dangerous and will potentially lead to bugs (3). Use the types of stdint.h instead.

To fix the code, you need to identify the loop iterator. It seems to be Cnt. So the for loop should be for(Cnt=1; Cnt<= Rbn_Pxl; Cnt++).

All other variable initializations are unrelated to the loop, and should be moved out. Similarly, there are various function calls that need to be inside the actual loop; they aren't in the slightest related to loop iteration.

Note: it isn't clear whether the intention was to declare 3 variables inside the loop, or to merely assign some values to them. I'll assume declaration was the intention.

int Bw_1 = (2 - Drctn) * (Drctn % 2);
int Bw_2 = (Drctn - 3) * ((Drctn - 1) % 2);

for(int Cnt=1; Cnt<= Rbn_Pxl; Cnt++)
{
  ...
  XFlush(Dsp);
  usleep(Usr_Draw_Arcs_Delay_Usec);
}

(But please change the variable names)

Sources:

  1. MISRA-C:2004 13.5 and 13.6.
  2. MISRA-C:2004 12.10
  3. MISRA-C:2004 6.3
share|improve this answer
    
There can be multiple iterators but there definitely should only be one. I've seen loops like for (int start=0, int end=10; start <= end; start++, end--) (I personally consider this as two iterators) –  Simon André Forsberg Jun 16 at 10:42
    
Do you mean for (int start=0, end=10; start <= end; start++, end--) ? –  Kevin Dong Nai Jia Jun 16 at 15:32
1  
@KevinDongNaiJia What does it matter, he gave an example of poor programming style. Don't dwell on it and don't write code like that. –  Lundin Jun 17 at 6:15
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.