C Functions Calling Other Functions With Side Effects

Recently, I've been working on a project and seeing a lot of this pattern:

void func(void) {
  retVal = thing1WithSideEffects();
  if (0 == retVal) thing2WithSideEffects();
  if (0 == retVal) thing3WithSideEffects()
  if (0 == retVal) thing4WithSideEffects();

  if (0 != retVal) return retVal;

  // do other func()y stuff
}

By side effect, I mean a change in the state of the system. The side effect might be a new open file, a memory allocation, the establishment of a TCP connection, whatever.

First, the style. The tests of retVal are actually checking things from the line above so reading this code is weird.
While I'm nitpicking, I hate "Yoda Conditions".

Way more importantly, however, is that if any one of the calls fails, the system is left in an indeterminate state. If retVal has a value by line 7, we have no idea what the failure was and we cannot call any cleanup functions to undo any of the side effects.

The calling program/function, detecting an error with func() cannot call func() again unless it knows how to clean things up.

Yes, getting this right is hard since you end up with something like this:

void func(void)
{
  if (retVal = thing1WithSideEffects()) return retVal;
  if (retVal = thing2WithSideEffects()) {
    undoThing1();
    return retVal;
  }
  if (retVal = thing3WithSideEffects()) {
    undoThing2();
    undoThing1();
    return retVal;
  }
  // ad nauseum -- you get the idea
}

A little bit gross (clean-up is beyond the scope of what I'm trying to get across) and hard to get-right/test.

I totally understand not wanting to do all this when you’re just under deadline, trying to get stuff working and out the door but the code we write tends to outlive us.

But we can and should do better.

Comments

[Andrew Clayton]

Linux Kernel coding style has you covered, slide 11 onwards...