Transitioning from C `goto` error handling paradigm to C++ exception handling paradigm

The principle of RAII is that you use a class type to manage any resource which needs cleaning up after use; that cleanup is done by the destructor.

That means you can create a local RAII manager, which will automatically clean up whatever it's managing when it goes out of scope, whether that's due to normal program flow or an exception. There should never be any need for a catch block just to clean up; only when you need to handle or report the exception.

In your case, you have three resources:

  • The file fp. ifstream is already an RAII type, so just remove the redundant calls to fp.close() and all is good.
  • The allocated memory data. Use a local array if it's a small fixed size (as this is), or std::vector if it needs to be dynamically allocated; then get rid of the delete.
  • The state set up by blackbox.

You can write your own RAII wrapper for the "black box" malarkey:

struct blackbox_guard {
    // Set up the state on construction
    blackbox_guard()  {blackbox();}

    // Restore the state on destruction
    ~blackbox_guard() {undo_blackbox();}

    // Prevent copying per the Rule of Three
    blackbox_guard(blackbox_guard const &) = delete;
    void operator=(blackbox_guard) = delete;
};

Now you can remove all your error-handling code; I'd indicate failure through exceptions (either thrown, or allowed to propagate) rather than a magic return value, giving:

void foobar(){
    ifstream fp ("blah.txt"); // No need to check now, the first read will fail if not open
    blackbox_guard bb;

    const size_t NUM_DATUM = 42;
    unsigned long data[NUM_DATUM];   // or vector<unsigned long> data(NUM_DATUM);

    for(size_t i = 0; i < NUM_DATUM; i++){
        string buffer;

        // You could avoid this check by setting the file to throw on error
        // fp.exceptions(ios::badbit); or something like that before the loop
        if(!getline(fp, buffer)){
             throw std::runtime_error("Failed to read"); // or whatever
        }

        stringstream(buffer) >> data[i]; // or data[i] = stoul(buffer);
    }

    for(size_t i = 0; i < NUM_DATUM/2; i++){
        cout << data[i] + data[i + NUM_DATUM/2] << endl;
    }
}

Yes, you should use RAII (Resource Acquisition Is Initialisation) wherever possible. It leads to code which is both easy to read and safe.

The core idea is that you acquire resources during the initialisation of an object, and set up the object so that it correctly releases the resources on its destruction. The vital point why this works is that destructors run normally when scope is exited via an exception.

In your case, there is already RAII available and you're just not using it. std::ifstream (I assume that's what your ifstream refers to) indeed closes on destruction. So all the close() calls in catch can safely be omitted and will happen automatically—precisely what RAII is for.

For data, you should be using an RAII wrapper too. There are two available: std::unique_ptr<unsigned long[]>, and std::vector<unsigned long>. Both take care of memory deallocation in their respective destructors.

Finally, for blackbox(), you can create a trivial RAII wrapper yourself:

struct BlackBoxer
{
  BlackBoxer()
  {
    blackbox();
  }

  ~BlackBoxer()
  {
    undo_blackbox();
  }
};

When rewritten with these, your code would become much simpler:

unsigned foobar() {
  ifstream fp ("blah.txt");
  if(!fp.is_open()){
    return 1;
  }

  try {
    BlackBoxer b;

    const size_t NUM_DATUM = 42;
    std::vector<unsigned long> data(NUM_DATUM);
    for(size_t i = 0; i < NUM_DATUM; i++){
      string buffer;
      if(!getline(fp, buffer)){
        return 1;
      }

      stringstream(buffer) >> data[i];
    }

    for(size_t i = 0; i < NUM_DATUM/2; i++){
      cout << data[i] + data[i + NUM_DATUM/2] << endl;
    }

    return 0;
  } catch (...) {
    return 1;
  }
}

Additionally, notice that your function uses a return value to indicate success or failure. This may be what you want (if failure is "normal" for this function), or might just represent only going half-way (if failure is supposed to be exceptional too).

If it's the latter, simply change the function to void, get rid of the trycatch construct, and throw a suitable exception instead of return 1;.

Finally, even if you decide to keep the return value approach (which is perfectly valid), consider changing the function to return bool, with true meaning success. It's more idiomatic.


Let me rewrite that for you using c++ idiom with explanations inline to the code

// void return type, we may no guarantees about exceptions
// this function may throw
void foobar(){
   // the blackbox function performs various
   // operations on, and otherwise modifies,
   // the state of external data structures
   blackbox();

   // scope exit will cleanup blackbox no matter what happens
   // a scope exit like this one should always be used
   // immediately after the resource that it is guarding is
   // taken.
   // but if you find yourself using this in multiple places
   // wrapping blackbox in a dedicated wrapper is a good idea
   BOOST_SCOPE_EXIT[]{
       undo_blackbox();
   }BOOST_SCOPE_EXIT_END


   const size_t NUM_DATUM = 42;
   // using a vector the data will always be freed
   std::vector<unsigned long> data;
   // prevent multiple allocations by reserving what we expect to use
   data.reserve(NUM_DATUM);
   unsigned long d;
   size_t count = 0;
   // never declare things before you're just about to use them
   // doing so means paying no cost for construction and
   // destruction if something above fails
   ifstream fp ("blah.txt");
   // no need for a stringstream we can check to see if the
   // file open succeeded and if the operation succeeded
   // by just getting the truthy answer from the input operation
   while(fp >> d && count < NUM_DATUM)
   {
       // places the item at the back of the vector directly
       // this may also expand the vector but we have already
       // reserved the space so that shouldn't happen
       data.emplace_back(d);
       ++count;
   }

   for(size_t i = 0; i < NUM_DATUM/2; i++){
       cout << data[i] + data[i + NUM_DATUM/2] << endl;
   }
}

The most powerful feature of c++ is not classes, it is the destructor. The destructor allows for resources or responsibilities to be discharged or released when the scope is exited. This means you don't have to re-write cleanup code multiple times. Moreover because only constructed objects can be destructed; if you never get to an item and thus never construct it, you do not pay any penalty in destruction if something happens.

If you find yourself repeating cleanup code, that should be a flag that the code in question is not taking advantages of the power of the destructor and RAII.