CppCoreGuidelines icon indicating copy to clipboard operation
CppCoreGuidelines copied to clipboard

Suggestion: promote better return type

Open MariusMyburg opened this issue 6 years ago • 11 comments

Good day. I want to make a suggestion. I am not 100% sure this is not covered in the guidelines already, but I do not see it upon quick inspection. Below is my argument and suggestion.

A function or method can fail. For example, let's take a simple example, fopen. As we all know, fopen may fail for a number of reasons.

fopen from cstdio looks like this:

FILE * fopen ( const char * filename, const char * mode );

This code implicitly 'expresses' that it can return a FILE pointer or nullptr in case of a problem. In my opinion, this is bad because meaning should be explicit, especially where it comes to success of failure.

So, to combat this, I encourage returning a simple templated type from all functions. For example, if I had to write fopen, I would have done it something like this:

// In order to facilitate conveying success and failure explicitly, I will use FunctionResult as a return value of all functions.
template<typename T> class FunctionResult
{
public:
    bool IsSuccess;
    T Result;

    FunctionResult()
    {
        IsSuccess = false;
        Result = nullptr;
    }
};

FunctionResult<FILE*> fopen_enhanced(const char * filename, const char * mode)
{
    FunctionResult<FILE*> result;

    FILE* ptrFile = fopen(filename, mode);
    if (ptrFile == nullptr)
    {
         result.IsSuccess = false;
    }else
    {
        result.IsSuccess = true;
        result.Result = ptrFile;
    }

    return result;
}

I have been using this mechanism, not only in C++ but in Kotlin as well, and it really adds to the expressiveness. I personally would make this practice a suggestion in the core guidelines, but I am open to criticism!

Thanks. Marius.

MariusMyburg avatar Feb 05 '19 15:02 MariusMyburg

Definitely a good idea. The std::expected<...> type may be used here, once it arrives. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4109.pdf

xtofl avatar Feb 05 '19 15:02 xtofl

What about std::optional ? In this case you don't have an object saying why the error happened (like in std::expected), so that's just as good as an optional.

Moreover, I think the example could be better constructed. Null pointers are a very common idiom in the language to say that "either you have the thing, or you dont", which is exactly what you need. I think that wrapping a pointer is overkill...

brenoguim avatar Feb 05 '19 17:02 brenoguim

E.2 actually uses fopen in an example...

cubbimew avatar Feb 05 '19 19:02 cubbimew

What about std::optional ? In this case you don't have an object saying why the error happened (like in std::expected), so that's just as good as an optional.

Moreover, I think the example could be better constructed. Null pointers are a very common idiom in the language to say that "either you have the thing, or you dont", which is exactly what you need. I think that wrapping a pointer is overkill...

I agree that the example may not have been the best possible because it is pretty well understood that a null pointer means failure, but it was just an example. I think the point I am trying to make is clear despite the example - FunctionResult is always better than returning just an int, FunctionResult<EMyEnum> is always better than returning just an enum, etc. Moreover, I thought about putting an 'Explanation' property in FunctionResult too; but still, my point was not really about everything that can be useful in such a type, only that such a type would almost always be better than to return just a basic type and to then rely on an implicit understanding (such as with pointers) rather making the intent explicit.

I do agree however, the example could be improved. Thanks for the suggestions. But I don't think my limited example fails to explain the core idea of my suggestion though.

MariusMyburg avatar Feb 05 '19 21:02 MariusMyburg

I didn't say (or mean) that the limited example fails to explain the core idea. The idea is very clear. It was just a small remark about the example...

But again, why not std::optional ?

brenoguim avatar Feb 05 '19 21:02 brenoguim

Understood, thanks.

I was not aware of std:: optional, I will check it out today. If there is an existing mechanism to express this, that is good. But more than commenting on the implementation details, I am more suggesting that the practice of using such a return type, whether custom or from the standard library, perhaps deserves an entry in the guidelines doc. I have seen too many people use out parameters, or have 'special' values for failure such as -1 when a method returns int, and I think the guidelines should state that returning a more explicitly clear type to indicate success or failure is preferred over having special cases. If std:: optional caters for this, that is great.

Anyway, thanks for pointing out std:: optional to me, I will look into it!

MariusMyburg avatar Feb 06 '19 06:02 MariusMyburg

By the way, the fact that pointer values are known to be possibly null_ptr is an idiom that should probably be stated explicitly, too.

xtofl avatar Feb 06 '19 13:02 xtofl

Editors call: This is touching on the general topic of recommendations for error handling so we'll keep issue open and consider it together with related issues in that broader context.

hsutter avatar Feb 14 '19 19:02 hsutter

Hi Herb. Thanks!

Cheers.

MariusMyburg avatar Feb 14 '19 19:02 MariusMyburg

Editors call: This is touching on the general topic of recommendations for error handling so we'll keep issue open and consider it together with related issues in that broader context.

Good day. I just noticed E.27 which specifically mentions this, down to an example with r.err and r.val being the error and value fields. Perhaps then this suggestion has been part of the Core Guidelines all along?

Cheers.

MariusMyburg avatar Mar 08 '19 13:03 MariusMyburg

I just noticed E.27 which specifically mentions this, down to an example with r.err and r.val being the error and value fields

There's nothing wrong with creating your own type with a member such as err to indicate an error condition, or using a std::tuple to do the same. But std::expected takes this to a whole new level. I've used Giovanni Dicanio's WinReg library for accessing the Windows Registry, which utilizes a wrapper similar to std::expected for returning results from functions, and found it a pleasure to write code with this approach. The examples on the WinReg GitHub page show how a Caller can handle a RegExpected result from a function, which evaluates as true if no error is contained within. The expected value or error value contained within the returned RegExpected object can be easily accessed.

std::expected provides this capability in a generic way, without requiring an extra library. It also adds fluent features ("monadic operations") if you wish to keep processing so log as no error state is encountered along the way (which can solve the goto exit issue as well). std::expected has its limitations, but its accessible in cpp23, as well as all the way back to cpp11 via Sy Brand's original tl::expected implementation (the tl::expected web page on GitHub illustrates how to ease error handling / goto exit scenarios). The implementation adopted in cpp23 has many of these monadic operations. std::expected could be mentioned in E.27 and perhaps other Core Guidelines topics that touch on error handling or returning multiple values.

Mike4Online avatar Feb 04 '23 07:02 Mike4Online