[ChuckMcM]

Interestingly I've been going back and forth on this lately. I have been playing around with SD cards on the STM32F4 and a typical SD Card transaction consists of 3 to 10 commands which, if any one fails, the transaction fails. I'm currently using negative conditionals of the form "Not Error" (the Error test is an affirmative, and so that seems ok to me)

A typical sequence is like the one to set the bus width.

   err = sdio_select(rca);
   if (! err) {
     err = sdio_command(55, rca << 16);
     if (! err) {
       err = sdio_command(6, 2);
     }
   }
   sdio_deslect();
   return err;

I had originally done it the other way

  err = sdio_select(rca);
  if (err) {
     return err;
  }
  err = sdio_command(55, rca << 16);
  if (err) {
      sdio_deselect();
      return err;
  }
  err = sdio_command(6, 2);
  sdio_deselect();
  return err;

I find the first form more readable. It also generates fewer branches in the generated code. The sample code I first looked at was doing Goto's to the exit code (deselect/return error) which was unacceptable :-).

In the original article the confusion arose around negative test cases and then testing for them negatively (double negatives) which I think are always bad from a readability point of view.

[nitrogen]

What I've found myself doing in such cases is adding a function that processes multiple commands and indicates an error if any one of the commands failed (sdio_batch could be a varargs function):

  struct sdio_cmd {
      enum { SDIO_SELECT, SDIO_COMMAND } cmd;
      int reg; // Guesses at suitable names without
      int val; // knowing anything about SDIO
  }

  int sdio_batch(int count, ...);
  
  err = sdio_batch(3,
      &(struct sdio_cmd){ SDIO_SELECT, .val = rca },
      &(struct sdio_cmd){ SDIO_COMMAND, 55, rca << 16 },
      &(struct sdio_cmd){ SDIO_COMMAND, 6, 2 },
      );

I've been doing it in Ruby with network remote procedure calls, where it's not quite as ugly to use inline lists and variable argument counts as it is in C, and where branch count isn't quite as important.

The sample code I first looked at was doing Goto's to the exit code (deselect/return error) which was unacceptable :-).

What's so bad about a little goto between friends?

[ChuckMcM]

This is a nice structure, It also suggests something like closures where you inverse stacked the functions and did something like:

   int do_command(sdio_command_chain *cmd) {
           int err = 0;
           if (cmd->next) {
              err = do_command(cmd->next);
           }
           return (err) ? err : call_command(cmd->parms);
     }

Thanks for that!

[Roboprog]

I actually prefer your second form: return as soon as you know you are done in the function/method -- if nothing more can be done, then don't pretend to do any more.

In the case of cleanup that must be done in the end, perhaps 2 functions would be better: a top level func to acquire and dispose of resources, calling an inner func to do as much work as it can with the resources. (assuming something like C that doesn't have a "finally" clause like Java)

I've never understood how finding the end of a long / nested mess of if/else blocks, rather than leaving the function, is somehow better. Which one feels more like a GOTO in terms of least astonishment?

[ChuckMcM]

   > I actually prefer your second form: return as soon as
   > you know you are done in the function/method -- if 
   > nothing more can be done, then don't pretend to do 
   > any more.

The first form actually does that. As soon as one if condition fails, the code falls through down to the bottom and returns. This is most impressive in the initialization/identification phase (see sdio_open() [1]) because that has like a dozen commands it has to get through successfully. By collapsing this way it allows for common cleanup, and if the cleanup requirements change you only have to change it in one place.

The effect of the first form (in C) is to collapse all of the if's "else" clauses into a single one at the bottom.

As for finding the other end of an off screen if clause, I agree with you, that it is a crutch to use the 'find matching' command in the editor with the open brace. The interesting about this problem is that it is driven by the SDIO spec, which was driven by a strict requirement of backwards compatibility, which results in very carefully crafted command / response / flow options.

This discussion has been helpful for me as at some point I am going to have to describe this to people new to, or possibly unfamiliar with, programming which should be interesting.

[1] https://github.com/ChuckM/stm32f4-sdio-driver/blob/master/sd...

[krzysz00]

Not to drag the thread off topic, but this seems like the kind of situation where a version of Haskell's Maybe or Either monads might come in handy, so you could write:

    do
      command1
      command2
      ...

and have the failures propagate automatically.

[cstavish]

One of the legitimate use cases for goto is for complex error handling. I'm not sure why it needs to be unacceptable across the board.