Seemingly pointless #define of function

Question

I've encountered some code along the lines of:

BOOL CBlahClass::SomeFunction(DWORD *pdw)
{
    RETURN_FALSE_IF_FILE_DOESNT_EXIST

    //the rest of the code makes sense...
    //...
}

Everything I see makes pretty good sense except I have a little question about the line RETURN_FALSE_IF_FILE_DOESNT_EXIST

I searched for this string and I find a #define:

#define RETURN_FALSE_IF_FILE_DOESNT_EXIST \
        if (FALSE==DoesFileExist()) return FALSE;

My question is... what the hell? Is there any good reason to make a #define like this? Why not just write:

BOOL CBlahClass::SomeFunction(DWORD *pdw)
{
    if ( FALSE == DoesFileExist() ) return FALSE

    //the rest of the code makes sense...
    //...
}

The only reason I can think of to do this is that it is a little bit easier and a little less annoying to write out "RETURN_FALSE_IF_FILE_DOESNT_EXIST" then to write out "if (FALSE==DoesFileExist()) return FALSE".

Anyone see any other reason to do this? Is there a name for this sort of thing?

Answer 1

There's no reason for it. A common way why programmers write such macros is because they have code with multiple-return statements (which should be avoided if possible - but sometimes it is just not sensible to avoid it). Example:

// Bad code

err_t func (void)
{
  err_t err;
  ...

  if(err == error1)
  {
    cleanup();
    return err;
  }

  ...

  if(err == error2)
  {
    cleanup();
    return err;
  }

  cleanup();
  return err;
}

To have the clean-up all over the function is bad practice, because of code repetition.

The ancient way of solving the problem more cleanly would be "on error goto", where you put a label at the end of the function. And do clean-up and return only there. It would actually be a somewhat acceptable solution, but goto is taboo so people shy away from it. Instead they come up with something like:

// Slightly less bad code

#define RETURN_FROM_FUNCTION(err) { cleanup(); return (err); }

err_t func (void)
{
  err_t err;
  ...

  if(err == error1)
  {
    RETURN_FROM_FUNCTION(err);
  }

  ...

  if(err == error2)
  {
    RETURN_FROM_FUNCTION(err);
  }

  RETURN_FROM_FUNCTION(err);
}

This removes code repetition, so the program is safer and easier to maintain. That's likely the answer to your question.

But this macro is still not a good solution, because it makes the code harder to read, because C programmers are good at reading C, but bad at reading the "home-brewn, secret macro language". And of course there's the usual concerns with macros: type safety, hard to debug/maintain etc.

However, all of the above mentioned methods (including on-error-goto) originate from an inability of thinking outside the box. The optimal solution is this:

// proper code

err_t func (void)
{
  err_t err;

  allocate(); // allocate whatever needs to be allocated

  err = func_internal();

  cleanup(); // one single place for clean-up
  return err;
}


static err_t func_internal (void) // local file scope, private function
{
  err_t err;
  ...

  allocate_if_needed(); // or maybe allocation needs to be here, based on results

  ...

  if(if(err == error1)
  {
    return err;
  }

  ...

  if(err == error2)
  {
    return err;
  }

  return err;
}

Answer 2

That macro is likely a guard clause that asserts a precondition. The precondition being that a certain file must exist because it doesn't make sense to call the function if it doesn't. The author probably wanted guard clauses to be noticeably different than "regular" checks and made it a big obvious macro. That it's a macro here is simply a style preference.

You write a guard clause when you require a condition to be true to continue. In a function called parseFile(), you might indeed expect the caller to have checked that the file exists. But in a function called openFile() it could be perfectly reasonable that the file does not exist yet (and therefore you wouldn't have a guard).

You may be more used to seeing assert(...). But what if you don't want your program to stop when the condition is false, and you want the condition checked even in the presence of NDEBUG? You could implement a macro like assert_but_return_false_on_fail(...), right? And that's what this macro is. An alternative assert().

glib is a very popular library that defines its precondition assertions like this:

#define g_return_if_fail()
#define g_return_val_if_fail()
#define g_return_if_reached
#define g_return_val_if_reached()
#define g_warn_if_fail()
#define g_warn_if_reached

The same code as yours in glib would be g_return_val_if_fail(DoesFileExist(), FALSE);.

The code you pasted could be better. Some problems are:

• It is very specific. Why not make it generic over any condition and not just whether a file exists?

• The return value is hard-coded into the name.

• It doesn't do further diagnostics. The glib functions log a rather detailed error on failure, including a stack trace so you can see the series of calls that led to the failure.

These problems are what make the macro seem silly and pointless. It would appear less pointless otherwise.

Or it's possible that the author was macro-obsessed and I'm reading too much into it.

Answer 3

Well, the idea behind using the macro is to simplify the maintenance in situations when this specific check might change. When you have a macro, all you have to change is the macro definition, instead of crawling through the whole program and changing each check individually. Also, it gives you the opportunity to completely eliminate this check by changing the macro definition.

In general, when one has a fairly well-rounded repetitive piece of code, one typically separates that code into a function. However, when that repetitive piece of code has to perform some unorthodox operation, like return that exits the enclosing function, macros are basically the only option. (One'd probably agree that tricks like that should be reserved to debugging/maintenance/system-level code, and should be avoided in the application-level code.)