C++ code purity

Question

I'm working in C++ enviroment and:
a) We are forbidden to use exceptions
b) It is application/data server code that evaluates lot of requests of different kinds

I have simple class encapsulating result of server operation that is also used internally for lot of functions there.

class OpResult
{
  .....
  bool succeeded();
  bool failed(); ....
  ... data error/result message ...
};

As I try to have all functions small and simple, lot of blocks like this are arising:

....
OpResult result = some_(mostly check)function(....);
if (result.failed())
  return result;
...

The question is, is it bad practise to make macro looking like this and use it everywhere?

#define RETURN_IF_FAILED(call) \
  {                            \
    OpResult result = call;    \
    if (result.failed())       \
      return result;           \
  }

I understand that someone can call it nasty, but is there a better way? What other way of handling results and avoiding lot of bloat code would you suggest?

Answer 1

After 10 years, I'm going to answer my own question to my satisfaction, if only I had a time machine ...

I encountered a similar situation many times in new projects. Even when exceptions were allowed, I don't want to always use them for "normal fails".

I eventually discovered a way to write these kind of statements.

For generic Result that includes message, I use this:

class Result
{
public:
  enum class Enum
  {
    Undefined,
    Meaningless,
    Success,
    Fail,
  };
  static constexpr Enum Undefined = Enum::Undefined;
  static constexpr Enum Meaningless = Enum::Meaningless;
  static constexpr Enum Success = Enum::Success;
  static constexpr Enum Fail = Enum::Fail;

  Result() = default;
  Result(Enum result) : result(result) {}
  Result(const LocalisedString& message) : result(Fail), message(message) {}
  Result(Enum result, const LocalisedString& message) : result(result), message(message) {}
  bool isDefined() const { return this->result != Undefined; }
  bool succeeded() const { assert(this->result != Undefined); return this->result == Success; }
  bool isMeaningless() const { assert(this->result != Undefined); return this->result == Enum::Meaningless; }
  bool failed() const { assert(this->result != Undefined); return this->result == Fail; }
  const LocalisedString& getMessage() const { return this->message; }

private:
  Enum result = Undefined;
  LocalisedString message;
};

And then, I have a special helper class in this form, (similar for other return types)

class Failed
{
public:
  Failed(Result&& result) : result(std::move(result)) {}
  explicit operator bool() const { return this->result.failed(); }
  operator Result() { return this->result; }
  const LocalisedString& getMessage() const { return this->result.getMessage(); }

  Result result;
};

When these are combined, I can write code like this:

if (Failed result = trySomething())
  showError(result.getMessage().str());

Isn't it beutiful?

Answer 2

Actually, I prefer slightly other solution. The thing is that the result of inner call is not necessarily a valid result of an outer call. For example, inner failure may be "file not found", but the outer one "configuration not available". Therefore my suggestion is to recreate the OpResult (potentially packing the "inner" OpResult into it for better debugging). This all goes to the direction of "InnerException" in .NET.

technically, in my case the macro looks like

#define RETURN_IF_FAILED(call, outerresult) \
  {                                         \
    OpResult innerresult = call;            \
    if (innerresult.failed())               \
    {                                       \
        outerresult.setInner(innerresult);  \
        return outerresult;                 \
    }                                       \
  }

This solution requires however some memory management etc.

Some purist argue that having no explicit returns hinders the readability of the code. In my opinion however having explicit RETURN as a part of the macro name is enough to prevent confusion for any skilled and attentive developer.

---

My opinion is that such macros don't obfuscate the program logic, but on the contrary make it cleaner. With such a macro, you declare your intent in a clear and concise way, while the other way seems to be overly verbose and therefore error-prone. Making the maintainers parse in mind the same construct OpResult r = call(); if (r.failed) return r is wasting of their time.

An alternative approach without early returns is applying to each code line the pattern like CHECKEDCALL(r, call) with #define CHECKEDCALL(r, call) do { if (r.succeeded) r = call; } while(false). This is in my eyes much much worse and definitely error-prone, as people tend to forget about adding CHECKEDCALL() when adding more code.

Having a popular need to do checked returns (or everything) with macros seems to be a slight sign of missing language feature for me.

Answer 3

As long as the macro definition sits in an implementation file and is undefined as soon as unnecessary, I wouldn't be horrified.

// something.cpp

#define RETURN_IF_FAILED() /* ... */

void f1 () { /* ... */ }
void f2 () { /* ... */ }

#undef RETURN_IF_FAILED

However, I would only use this after having ruled out all non-macro solutions.

Answer 4

It's a trade off. You are trading code size for obfuscation of the logic. I prefer to preserve the logic as visible.

I dislike macros of this type because they break Intellisense (on Windows), and debugging of the program logic. Try putting a breakpoint on all 10 return statements in your function - not the check, just the return. Try stepping through the code that's in the macro.

The worst thing about this is that once you accept this it's hard to argue against the 30-line monster macros that some programmers LOVE to use for commonly-seen mini-tasks because they 'clarify things'. I've seen code where different exception types were handled this way by four cascading macros, resulting in 4 lines in the source file, with the macros actually expanding to > 100 real lines. Now, are you reducing code bloat? No. It's impossible to tell easily with macros.

Another general argument against macros, even if not obviously applicable here, is the ability to nest them with hard to decipher results, or to pass in arguments that result in weird but compilable arguments e.g. the use of ++x in a macros that uses the argument twice. I always know where I stand with the code, and I can't say that about a macro.

EDIT: One comment I should add is that if you really do repeat this error check logic over and over, perhaps there are refactoring opportunities in the code. Not a guarantee but a better way of code bloat reduction if it does apply. Look for repeated sequences of calls and encapsulate common sequences in their own function, rather than addressing how each call is handled in isolation.