Good C-coding style for multiple lines if conditions

Question

I am programming a project in C and have a problem: I've got a lot of if conditions and it might get really hard for other people to read. I haven't yet found a similar question on the internet.

Do you have an idea or example how to make my code more readable?

Here is the C Code:

if( ((g_cycle_cnt == uartTxSecondaryMsg[3][msgPos[3]].sliceNo) ||   //correct slicenumber...
    (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -1) ||             // or as fast as possible...                                            

  ( (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -2) &&
   ((uartTxSecondaryMsg[3][msgPos[3]].timeFrameBegin>=g_uptime_cnt) && 
    (uartTxSecondaryMsg[3][msgPos[3]].timeFrameEnd<=g_uptime_cnt)))) &&

   ((dataProcessingFlag & SECONDARY_MSG_ANNOUNCED_CH4) == SECONDARY_MSG_ANNOUNCED_CH4) )

"*Do you have an idea or example how to make it more readable?*" Use a proper indention? — alk, Mar 29 '15 at 12:23
That seemed to be a problem from copying it from eclipse to this window.. I just realized the mistake after it was already posted. — En Kt, Mar 29 '15 at 12:32
@abligh: Ah well ... alright so 1+ for asking to learn how to clean up! ;-) — alk, Mar 29 '15 at 12:40
@EnKt: "*I just realized the mistake after it was already posted.*" So what about fixing it? — alk, Mar 29 '15 at 12:41

Scis · Answer 1 · 2015-03-29T12:27:17.040

12

Create functions that have indicative names that check the requirements and represent their meaning e.g:

if( is_correct_slice_number(/*... params here ... */) || 
    is_asap(/*... params here ... */)  || 
    is_other_condition(/*... params here ... */))

Or as suggested macros that follow the same logic e.g:

if( IS_CORRECT_SLICE_NUMBER(/*... params here ... */) || 
    IS_ASAP(/*... params here ... */)  || 
    IS_OTHER_CONDITION(/*... params here ... */))

I think that this might make your intentions clearer.

edited Mar 29 '15 at 12:27

answered Mar 29 '15 at 12:20

Scis

2,934
3
23
37

1+, however, macros would do also. – alk Mar 29 '15 at 12:24
actually the collection of the 3 checks themselves must have some kind of meaning too and can be encapsulated in a more descriptive name. – Philip Stuyck Mar 29 '15 at 12:35
Thank you! I will think about whether it would be applicable or not in this case. – En Kt Mar 29 '15 at 12:36

abligh · Accepted Answer · 2015-03-29T12:28:50.513

If you want to stick with your existing code (as opposed to factor things out into inline functions), and just modify the indentation, I'm a big fan of using indent consistently. This means you can fix any source file.

With its default options it gives you GNU indentation, i.e.:

if (((g_cycle_cnt == uartTxSecondaryMsg[3][msgPos[3]].sliceNo) ||       //correct slicenumber...
     (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -1) ||        // or as fast as possible...
     ((uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -2) &&
      ((uartTxSecondaryMsg[3][msgPos[3]].timeFrameBegin >= g_uptime_cnt) &&
       (uartTxSecondaryMsg[3][msgPos[3]].timeFrameEnd <= g_uptime_cnt)))) &&
    ((dataProcessingFlag & SECONDARY_MSG_ANNOUNCED_CH4) ==
     SECONDARY_MSG_ANNOUNCED_CH4))
  {
    /* do something */
  }

I'd say that the problem here is in fact that you are poking about illegibly in arrays. At the very least, factor out:

uartTxSecondaryMsg[3][msgPos[3]]

into a separate variable, e.g.:

whatever *foo = &uartTxSecondaryMsg[3][msgPos[3]];
if (((g_cycle_cnt == foo->sliceNo) ||   //correct slicenumber...
     (foo->sliceNo == -1) ||    // or as fast as possible...
     ((foo->sliceNo == -2) &&
      ((foo->timeFrameBegin >= g_uptime_cnt) &&
       (foo->timeFrameEnd <= g_uptime_cnt)))) &&
    ((dataProcessingFlag & SECONDARY_MSG_ANNOUNCED_CH4) ==
     SECONDARY_MSG_ANNOUNCED_CH4))
  {
    /* do something */
  }

Obviously choose an appropriate type and variable name for foo.

You could then separate out the limbs of the if statement into separate functions each taking foo as a parameter.

That seems to be a great idea! thank you very much! I will use your idea with the pointer straight away. :) — En Kt, Mar 29 '15 at 12:37

wildplasser · Answer 3 · 2015-03-29T13:25:40.197

[this is very subjective]

remove excessive parentheses; they are defensive and obscure the meaning
properly align and order the conditions (possibly ignoring indentation rules)
(maybe) rewrite into another construct, such as a switch, or using early returns, or even goto

First step (cleanup and alignment):

if (    (  uartTxSecondaryMsg[3][msgPos[3]].sliceNo == g_cycle_cnt //correct slicenumber...
        || uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -1          // or as fast as possible...                                
        ||(uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -2
             && uartTxSecondaryMsg[3][msgPos[3]].timeFrameBegin >= g_uptime_cnt
             && uartTxSecondaryMsg[3][msgPos[3]].timeFrameEnd <= g_uptime_cnt
             && (dataProcessingFlag & SECONDARY_MSG_ANNOUNCED_CH4) == SECONDARY_MSG_ANNOUNCED_CH4 ) ) 
                {
                // do something useful here
                }

Second step, using switch (and goto !) [this could be a bit too much for some readers ...]

switch (uartTxSecondaryMsg[3][msgPos[3]].sliceNo) {
default:
    if (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == g_cycle_cnt) goto process;
    break;
case -2:
    if (uartTxSecondaryMsg[3][msgPos[3]].timeFrameBegin < g_uptime_cnt) break;
    if (uartTxSecondaryMsg[3][msgPos[3]].timeFrameEnd > g_uptime_cnt) break;
    if ((dataProcessingFlag & SECONDARY_MSG_ANNOUNCED_CH4) != SECONDARY_MSG_ANNOUNCED_CH4) break;
case -1:
process:

          // do something useful here
 }

The advantage of the switch() construct is that it immediately clear that uartTxSecondaryMsg[3][msgPos[3]].sliceNo is the master condition. Another advantage is that cases and conditions can be added without having to interfere with the other cases or to struggle with the parentheses.

Now I hope I got the parentheses-removal right...

My colleagues would kill me for using goto.. but thank you for showing me an example of an good alignment. — En Kt, Mar 29 '15 at 13:21
I understand that both the alignment and the goto will probably violate coding-style guides. But to some people, readability is more important. — wildplasser, Mar 29 '15 at 13:31
The indentation and placing the logical operators at the beginning of the line is a very good way to give overview of the order of the conditions. (I use it myself often too. I upvoted this answer.) The switch is obfuscating and does not give me the overview (I should now downvote the answer again). However, the '{' and '}' should be at the normal indentation level again, i.e. one tabstop indented from the column of 'if' because the action doesn _not_ start when the 3rd OR is true (but when any OR is true). — Paul Ogilvie, Mar 29 '15 at 13:53
[I could have screwed up, removing the `{}`s] The switch in this case *is* over the top here, but on protocol-machines like this it can come in handy to separate (or extend) the cases. — wildplasser, Mar 29 '15 at 14:04

Wayne Taylor · Answer 4 · 2019-09-26T14:28:35.707

This is a great example of why indentation is so important. As I was reading through other people's solutions, I realized some people got the last && clause wrong. When I used Notepad++ to highlight the parens, I discovered that the last && clause was actually at the highest level, whereas most of the re-writes had it paired with "....timeFrameEnd<=g_uptime_cnt"

Also, looking at the third clause notice that (test_1 && (test_2 && test_3)) is equivalent to (test_1 && test_2 && test_3), so some of the complication can be cleaned up by just removing a layer of parens.

I prefer Example C.

// Example A:
if (  (  (g_cycle_cnt == uartTxSecondaryMsg[3][msgPos[3]].sliceNo)  //correct slicenumber
      || (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -1)           // or as fast as possible...
      || (  (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -2)
         && (  (uartTxSecondaryMsg[3][msgPos[3]].timeFrameBegin>=g_uptime_cnt)
            && (uartTxSecondaryMsg[3][msgPos[3]].timeFrameEnd<=g_uptime_cnt))))
      && ((dataProcessingFlag & SECONDARY_MSG_ANNOUNCED_CH4) == SECONDARY_MSG_ANNOUNCED_CH4)
   ){
    // do something
}

// Example B
if ( ( (g_cycle_cnt == uartTxSecondaryMsg[3][msgPos[3]].sliceNo) ||   //correct slicenumber...
       (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -1) ||             // or as fast as possible...
       ( (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -2) &&
         ( (uartTxSecondaryMsg[3][msgPos[3]].timeFrameBegin >= g_uptime_cnt) &&
           (uartTxSecondaryMsg[3][msgPos[3]].timeFrameEnd <= g_uptime_cnt)))) &&

     ( (dataProcessingFlag & SECONDARY_MSG_ANNOUNCED_CH4) == SECONDARY_MSG_ANNOUNCED_CH4) 
   )
{
    // do something
}

// Example C
if( ( (g_cycle_cnt == uartTxSecondaryMsg[3][msgPos[3]].sliceNo) ||   //correct slicenumber...
      (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -1) ||             // or as fast as possible...
      ( (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -2) &&
        (uartTxSecondaryMsg[3][msgPos[3]].timeFrameBegin>=g_uptime_cnt) && 
        (uartTxSecondaryMsg[3][msgPos[3]].timeFrameEnd<=g_uptime_cnt)
      )
    ) &&
    ( (dataProcessingFlag & SECONDARY_MSG_ANNOUNCED_CH4) == SECONDARY_MSG_ANNOUNCED_CH4) 
  )
{
    // do something
}

score 1 · Answer 5 · edited Sep 20 '19 at 22:01

1

This is really subjective (i.e. there is almost as many opinions on how to make this sort of thing better as there are people).

I'd probably use a couple of extra variables, such as

WhateverType  your_object = uartTxSecondaryMsg[3][msgPos[3]];
int slice = your_object.sliceNo;
int begin = your_object.timeFrameBegin;
int end = your_object.timeFrameEnd;

if ( ( g_cycle_cnt == slice || 
       slice == -1 ||
       (slice == -2 && begin >= g_uptime_cnt && end <= g_uptime_cnt)
     ) &&
     ((dataProcessingFlag & SECONDARY_MSG_ANNOUNCED_CH4) == SECONDARY_MSG_ANNOUNCED_CH4) 
   )

edited Sep 20 '19 at 22:01

Wayne Taylor

71
4

answered Mar 29 '15 at 12:29

Peter

35,646
4
32
74

1+ for "*This is really subjective*" It might be worth mentioning that in the context of "programming" the scheme used to layout should at least follow a system, which can be apdapted by the reader. – alk Mar 29 '15 at 12:43

score 0 · Answer 6 · answered Dec 09 '20 at 19:10

With function design you can do something like this. If you want you can also use else if structure so that code is tighter but I prefer this. This way horizontal code is minimal. Maybe I have watch too much Linux kernel code. But I think this is the way someone in kernel space can make this.

Your if is so monsteres that nobody could ever follow it. This style you can check line by line and also add comments before if statements if needed.

void foo()
{
    if (dataProcessingFlag & SECONDARY_MSG_ANNOUNCED_CH4 != SECONDARY_MSG_ANNOUNCED_CH4)
        return;

    if (g_cycle_cnt == uartTxSecondaryMsg[3][msgPos[3]].sliceNo)
        goto out;

    if (uartTxSecondaryMsg[3][msgPos[3]].sliceNo != -2)
        return;

    if (uartTxSecondaryMsg[3][msgPos[3]].timeFrameBegin < g_uptime_cnt)
        return;

    if (uartTxSecondaryMsg[3][msgPos[3]].timeFrameEnd > g_uptime_cnt)
        return;
out:
    // Do something.

    // Another way is to call another function here and also in goto statement
    // That way you don't have to use goto's if do not want.
}

Good C-coding style for multiple lines if conditions

6 Answers6