managing if statements

gcc 4.4.3 c89

I have some functions that initialize some hardware and return either true or false. If false then I have to uninitialize in the reverse order.

However, my code is looking very untidy with all the if statements.

For example each function can return either true of false. This is a sample. As you can see the code looks very untidy. I am just looking for any advice on how I can clean it up to make it more manageable and if possible scable?

Many thanks for any advice,

if(init_A() == TRUE) {
 if(init_B() == TRUE) {
  if(init_C() == TRUE) {
   if(init_D() == TRUE) {
    if(init_E() == TRUE) {
     /* ALL STARTED OK */    
    }
    else {
     uninit_A();
     uninit_B();
     uninit_C();   
     uninit_D();    
    }
   }
   else {
    uninit_A();
    uninit_B();
    uninit_C();   
   }
  }
  else {
   uninit_A();
   uninit_B();
  }
 }
 els开发者_Go百科e {
  /* Failed to initialize B */
  uninit_B(); 
 }
}
else {
 /* Failed to start */
}

---

if(init_A() != TRUE) {
    goto EndA;
}
if(init_B() != TRUE) {
    goto EndB;
}
if(init_C() != TRUE) {
    goto EndC;
} 
if(init_D() != TRUE) {
    goto EndD;
}
if(init_E() != TRUE) {
    goto EndE;
} 
...
return;
EndE: uninitD();
EndD: uninitC();
EndC: uninitB();
EndB: uninitA();
EndA: return;

---

This is quite a common problem, where the "init" steps correspond to things like malloc() or lock(), and the "uninit" steps correspond to things like free() and unlock(). It is particularly an issue when resources have to be deallocated in strictly the reverse order in which they were allocated.

This is one case where the use of goto is justified:

int somefunc()
{
    int retval = ERROR;

    if (init_A() != TRUE)
        goto out_a;

    if (init_B() != TRUE)
        goto out_b;

    if (init_C() != TRUE)
        goto out_c;

    if (init_D() != TRUE)
        goto out_d;

    if (init_E() != TRUE)
        goto out_e;

    /* ALL STARTED OK */
    /* ... normal processing here ... */
    retval = OK;

    uninit_E();
  out_e:
    uninit_D();
  out_d:
    uninit_C();
  out_c:
    uninit_B();
  out_b:
    uninit_A();
  out_a:
    return retval;
}

---

I would loop through an array of function pointers, call the functions in the loop, then if that function returned false, perform the corresponding uninit_* function.

Here's an example:

void (*inits[5]) (void);
void (*uninits[4]) (void);

int main(void) {
   inits[0] = init_A;
   inits[1] = init_B;
   inits[2] = init_C;
   inits[3] = init_D;
   inits[4] = init_E;

   uninits[0] = uninit_A;
   uninits[1] = uninit_B;
   uninits[2] = uninit_C;
   uninits[3] = uninit_D;

   for(int i = 0; i < 5; i++) {
      if((*inits[i])() != TRUE) {
         int j = (i < 4) ? i : 4; 
         while(j--) {
             (*uninits[j])();
         }
         break;
      }
   }
   return 1;
}

---

BOOL a = FALSE, b = FALSE, c = FALSE, d = FALSE, e = FALSE;

if ( (a = init_A())  &&  (b = init_B())  &&  (c = init_C())  && (d = init_D())  && (e = init_E()) )
{
}
else
{
    if ( e ) uninit_E();
    if ( d ) uninit_D();
    if ( c ) uninit_C();
    if ( b ) uninit_B();
    if ( a ) uninit_A();
}

uninit functions are called in direct order, as in your code. If reverse order is required, just change this.

---

If your uninit_* functions can detect whether or not they need to do anything you can simply:

if (!init_A() || !init_B() || !init_C() || !init_D() )
{
  uninit_C();
  uninit_B();
  uninit_A();
  return FALSE;
}

---

Is that "reverse order"? For me reverse order is like this:

void uninit(int from) {
    switch (from) {
        /* ... */
        case 3: uninit_C(); /* fall_through */
        case 2: uninit_B(); /* fall_through */
        case 1: uninit_A(); /* fall_through */
        case 0: break;
    }
}

And the init process would go like this

    int count = 0;
    if (init_A()) {
        count++;
        if (init_B()) {
            count++;
            if(init_C()) {
                count++;
                if(init_D()) {
                    count++;
                    if(init_E()) {
                        count++;
                    }
                }
            }
        }
    }
    if (count == 5) /* ALL OK */;
    uninit(count);

---

Limited understanding of C at work here, if you do decide to downvote, please tell me why.

#include <stdio.h>

int init_a() { return 1; }; // succeed
int init_b() { return 1; }; // succeed
int init_c() { return 0; }; // fail

void uninit_a() { printf("uninit_a()\n"); }
void uninit_b() { printf("uninit_b()\n"); }
void uninit_c() { printf("uninit_c()\n"); }

typedef struct _fp {
        int (*init)();
        void (*uninit)();
} fp;

int init() {
        fp fps[] = {
                (fp){&init_a, &uninit_a},
                (fp){&init_b, &uninit_b},
                (fp){&init_c, &uninit_c}
        };

        unsigned int i = 0, j;
        for(; i < sizeof(fps) / sizeof(fp); ++i) {
                if(!(*fps[i].init)()) {
                        for(j = 0; j < i; ++j) {
                                (*fps[j].uninit)();
                        }
                        return -1;
                }
        }
        return 0;
}

int main() {
        init();
        return 0;
}

Output:

uninit_a()
uninit_b()

This is the same order that the code in original post would be executed in, but you may want to reverse it (inner loop).

---

What you perhaps are looking for is "scope bound resource management". C++ traditionally does that with constructors/destructors. But there is a way to do that differently (in C99 as well as in C++) by abusing the for-statement a bit. I wrote something up upon this line here: scope bound resource management with for scopes.

---

I've not got a compiler to try this out. But something like this might work?

int (*init[])() = {init_A, init_B, init_C, init_D, init_E};
int (*uninit[])() = {uninit_A, uninit_B, uninit_C, uninit_D, uninit_E};

int main()
{
  initfunction(init, 0)
  return 0;
}

void initfunction((*init[])(), pos)
{
  if(init[pos]() == TRUE)
    initfunction(init, pos++)
  else
    return;

  uninit[pos]();
}

---

int X = 0;
if(init_A() == TRUE) {
  X++;
  if(init_B() == TRUE) {
    X++;
    if(init_C() == TRUE) {
      X++;
      if(init_D() == TRUE) {
        X++;
        if(init_E() == TRUE) {
          X++;
          /* ALL STARTED OK */    
        }
      }
    }
  }
}

/* You said reverse order which I took to mean this,
 * though your did not do it this way. */
switch (X) {
  case 5:
      return SUCCESS;
  case 4:
    uninit_D();
  case 3:
    uninit_C();
  case 2:
    uninit_B();
  case 1:
    uninit_A();
    return FAILURE;
}

Something I find myself doing to prevent myself from making errors in code like this is:

static int do_A(void);
static int do_B(void);
static int do_C(void);
static int do_D(void);

static int do_A(void) {
   if (init_A() == FALSE) {
      return FALSE;
   }
   if (do_B() == FALSE) {
      uninit_A();
      return FALSE;
   }
   return TRUE;
}    

...

static int do_D(void) {
    return init_D();
}

All of the other do_ functions should look similar to do_A.