CODEWITHSUNDEEP
c
ant2009
ant2009

Reputation: 22486

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();
  }
 }
 else {
  /* Failed to initialize B */
  uninit_B(); 
 }
}
else {
 /* Failed to start */
}

Upvotes: 10

Views: 1347

Answers (10)

Alex F
Alex F

Reputation: 43311

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.

Upvotes: 6

Jacob Relkin
Jacob Relkin

Reputation: 163228

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

Upvotes: 7

adamk
adamk

Reputation: 46794

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;

Upvotes: 24

nategoose
nategoose

Reputation: 12382

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.

Upvotes: 1

Jens Gustedt
Jens Gustedt

Reputation: 78903

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.

Upvotes: 2

pmg
pmg

Reputation: 108968

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

Upvotes: 4

Steven Keith
Steven Keith

Reputation: 1799

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]();
}

Upvotes: 1

Daniel Sloof
Daniel Sloof

Reputation: 12706

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

Upvotes: 2

caf
caf

Reputation: 239011

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

Upvotes: 8

Matthew
Matthew

Reputation: 48284

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

Upvotes: 4

Related Questions