C++ code purity

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 l开发者_如何学Goike 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?

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.

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.

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.

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?

I agree with Steve's POV.

I first thought, at least reduce the macro to

#define RETURN_IF_FAILED(result) if(result.failed()) return result;

but then it occurred to me this already is a one-liner, so there really is little benefit in the macro.

I think, basically, you have to make a trade off between write-ability and readability. The macro is definitely easier to write. It is, however, an open question whether it is also is easier to read. The latter is quite a subjective judgment to make. Still, using macros objectively does obfuscate code.

Ultimately, the underlying problem is that you must not use exceptions. You haven't said what the reasons for that decision are, but I surely hope they are worth the problems this causes.

Could be done with C++0x lambdas.

template<typename F> inline OpResult if_failed(OpResult a, F f) {
    if (a.failed())
        return a;
    else
        return f();
};

OpResult something() {
    int mah_var = 0;
    OpResult x = do_something();
    return if_failed(x, [&]() -> OpResult {
        std::cout << mah_var;
        return f;
    });
};

If you're clever and desperate, you could make the same kind of trick work with regular objects.

In my opinion, hiding a return statement in a macro is a bad idea. The 'code obfucation' (I like that term..! ) reaches the highest possible level. My usual solution to such problems is to aggregate the function execution at one place and control the result in the following manner (assuming you have 5 nullary functions):

std::array<std::function<OpResult ()>, 5>  tFunctions = {
 f1, f2, f3, f4, f5
};

auto tFirstFailed = std::find_if(tFunctions.begin(), tFunctions.end(), 
    [] (std::function<OpResult ()>& pFunc) -> bool {
        return pFunc().failed();
    });

if (tFirstFailed != tFunctions.end()) {
 // tFirstFailed is the first function which failed...
}

Is there any information in result which is actually useful if the call fails?

If not, then

static const error_result = something;

if ( call().failed() ) return error_result; 

would suffice.