CODEWITHSUNDEEP
cif-statementconditional-statementslines
En Kt
En Kt

Reputation: 113

Good C-coding style for multiple lines if conditions

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) )

Upvotes: 8

Views: 26224

Answers (7)

Pxtl
Pxtl

Reputation: 964

I'm going to assume you've put parentheses and linebreaks generally where you want them, and not mess with that. I think model the parse-tree as the indentation tree with OTBS, but whether one-line a given node or multiline it is developer prference, with the understanding that you either you multiline each child, oneline all children.

My rules:

  1. If it's worth multilining an expression, it gets the OTBS treatment. Open at the end of the first line, dedented close at the end of the block parent object.
  2. Infix operators go at the start of line.
  3. You're allowed to jump multiple levels in a single line, only need one layer of indent, but the number or of levels you go in must match the number of levels you go out at the end-line.

The logic:

  1. For lines that are peers in the parse-tree, the relationship of a line to the line above it is always at the start of the line.
  2. For parents/children, the relationship between the first child and its parent is at the end of the parent line.
  3. For endings, they always stand alone.
  4. The end of an indented section is always at the same level of indentation as the start of that section.
if( 
  (
    //correct slicenumber...
    (g_cycle_cnt == uartTxSecondaryMsg[3][msgPos[3]].sliceNo)
    // or as fast as possible... 
    || (uartTxSecondaryMsg[3][msgPos[3]].sliceNo == -1) 
    || (
      (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
}

Now, if this were my code I'd also look at reducing some of the redundant parentheses and thus flattening the tree a bit, and also I'd be storing some repeated bits into variables (particularly the uartTxSecondaryMsg[3][msgPos[3] part), but for the indentation? Yes, that sort of "OTBS but for parentheses" and "put the infix operators at the beginning" is my go-to style.

Upvotes: 0

teksturi
teksturi

Reputation: 99

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.
}

Upvotes: 0

Wayne Taylor
Wayne Taylor

Reputation: 51

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
}

Upvotes: 1

Peter
Peter

Reputation: 36617

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) 
   )

Upvotes: 1

wildplasser
wildplasser

Reputation: 44250

[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...

Upvotes: 3

abligh
abligh

Reputation: 25169

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.

Upvotes: 7

Scis
Scis

Reputation: 2984

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.

Upvotes: 12

Related Questions