Result class to get either value or error

Question

Recently, I had written a Result class. Basically I wanted a function to return result value on success and some error code on failure.

enum class ErrorCode : std::uint8_t
{
    OK,
    INVALID_PARAMETERS,
    DATA_UNAVAILABLE,
    UNKNOWN_ERROR
};

template <typename ValueType>
class Result
{
    const ValueType m_value;
    const ErrorCode m_error;
    const bool m_has_error;

public:
    static_assert(std::is_copy_constructible<ValueType>::value && std::is_copy_assignable<ValueType>::value, "ValueType must be both copy-constructible and copy-assignable");
    static_assert(std::is_move_constructible<ValueType>::value && std::is_move_assignable<ValueType>::value, "ValueType must be both move-constructible and move-assignable");

    explicit Result(ErrorCode e) noexcept : m_value{}, m_error{e}, m_has_error{true}  {}

    explicit Result(ValueType t) noexcept : m_value{t}, m_error{ErrorCode::OK}, m_has_error{false}  {}

    [[nodiscard]] bool has_value() const noexcept { return !m_has_error; }

    [[nodiscard]] bool has_error() const noexcept { return m_has_error; }

    [[nodiscard]] ValueType value() const noexcept { return m_value; }

    [[nodiscard]] ErrorCode error() const noexcept { return m_error; }
};

This is an example of how one may use it:

// Dummy function which returns sample data
ErrorCode fetch_data_from_server(std::vector<int>& data)
{
    static int i = 0;
    if ((++i) % 2 == 0)
    {
        data.push_back(23);
        return ErrorCode::OK;
    }
    return ErrorCode::DATA_UNAVAILABLE;
}

Result<std::vector<int>> get_data()
{
    using Res = Result<std::vector<int>>;

    std::vector<int> result;
    auto ec = fetch_data_from_server(result);
    if (ec != ErrorCode::OK)
    {
        return Res(ec);
    }
    return Res(result);
}

int main()
{
    auto res = get_data();
    if (res.has_error())
    {
        std::cout << "Error: " << static_cast<int>(res.error()) << '\n';
    }
    else
    {
        std::cout << "Data has size: " << res.value().size() << '\n';
    }
}

Please provide some feedback.

Answer 1

Design review

The idea is good, and in fact has been implemented multiple times by experienced coders, starting (I think) with Alexandrescu’s Expected. There’s also Niall Douglas’s Outcome, and of course, now we have std::expected.

Ergonomics

I think it is worthwhile to see what those types can do, compared to yours. This is what your sample would look like with std::expected:

auto get_data() -> std::expected<std::vector<int>, ErrorCode>
{
    std::vector<int> result;
    if (auto ec = fetch_data_from_server(result); ec != ErrorCode::OK)
        return std::unexpected{ec};

    return result;
}

int main()
{
    if (auto res = get_data(); not res)
    {
        std::cout << "Error: " << static_cast<int>(res.error()) << '\n';
    }
    else
    {
        std::cout << "Data has size: " << res->size() << '\n';
    }
}

For this simple case, it’s mostly the same, but there are a few features of std::expected that are more ergonomic.

Because the conversion from the result type to Result is explicit, the return has to be return Result<std::vector<int>>{result}; (which you simplify by means of a type alias). With std::expected, the conversion is implicit, so no type aliases or casting is required.

Another neat feature of std::expected is the pointer-like semantics, which is natural and less verbose than manually calling .has_value() or .value() (though you have that option, too).

But the real power of std::expected comes when the example gets more complex. Consider this situation: imagine you want to get data from a server, compute some value from that data, check that the value is valid, use it to produce some output, and then print it.

This is what that might look like with Result:

auto get_data() -> Result<std::vector<int>>
{
    using Res = Result<std::vector<int>>;

    std::vector<int> result;
    auto ec = fetch_data_from_server(result);
    if (ec != ErrorCode::OK)
    {
        return Res(ec);
    }
    return Res(result);
}

auto compute_value(std::span<int const> data) -> Result<int>
{
    using Res = Result<int>;

    if (data.empty())
        return Res{ErrorCode::dataset_empty};

    if (data.size() > 10uz)
        return Res{ErrorCode::dataset_too_large};

    return Res{*std::ranges::fold_left_first(data, std::plus<int>())};
}

auto check_value(int value) -> Result<int>
{
    using Res = Result<int>;

    if (value < 5)
        return Res{ErrorCode::value_too_low};
    if (value > 8)
        return Res{ErrorCode::value_too_high};

    return Res{value};
}

auto generate_output(int value) -> Result<std::string>
{
    using Res = Result<std::string>;

    switch (value)
    {
    case 5:
        return Res{"five"};
    case 6:
        return Res{"six"};
    case 7:
        return Res{"seven"};
    case 8:
        return Res{"eight"};
    default:
        return Res{ErrorCode::unrecognized_value};
    }
}

auto main() -> int
{
    auto print_error = [](ErrorCode e) { std::cout << "Error: " << static_cast<int>(e) << '\n'; };

    auto res = get_data();
    if (res.has_value())
    {
        auto res1 = compute_value(res.value());
        if (res1.has_value())
        {
            auto res2 = check_value(res1.value());
            if (res2.has_value())
            {
                auto res3 = generate_output(res2.value());
                if (res3.has_value())
                {
                    std::cout << "Result: " << res3.value() << '\n';
                }
                else
                {
                    print_error(res3.error());
                }
            }
            else
            {
                print_error(res2.error());
            }
        }
        else
        {
            print_error(res1.error());
        }
    }
    else
    {
        print_error(res.error());
    }
}

Note that I actually had to extend ErrorCode to have more error cases. I wouldn’t have to do that with std::expected, because I could use any type I please as the error type. But for a fairer comparison, I’ll use the same error type for both.

And this is what it might look like with std::expected:

template<typename T>
using Result = std::expected<T, ErrorCode>;

auto get_data() -> Result<std::vector<int>>
{
    std::vector<int> result;
    if (auto ec = fetch_data_from_server(result); ec != ErrorCode::OK)
        return std::unexpected{ec};
 
    return result;
}

auto compute_value(std::span<int const> data) -> Result<int>
{
    if (data.empty())
        return std::unexpected{ErrorCode::dataset_empty};

    if (data.size() > 10uz)
        return std::unexpected{ErrorCode::dataset_too_large};

    return *std::ranges::fold_left_first(data, std::plus<int>());
}

auto check_value(int value) -> Result<int>
{
    if (value < 5)
        return std::unexpected{ErrorCode::value_too_low};
    if (value > 8)
        return std::unexpected{ErrorCode::value_too_high};

    return value;
}

auto generate_output(int value) -> Result<std::string>
{
    switch (value)
    {
    case 5:
        return "five";
    case 6:
        return "six";
    case 7:
        return "seven";
    case 8:
        return "eight";
    default:
        return std::unexpected{ErrorCode::unrecognized_value};
    }
}

auto main() -> int
{
    get_data()
        .and_then(compute_value)
        .and_then(check_value)
        .and_then(generate_output)
        .and_then([](auto&& output)
        {
            std::cout << "Result: " << output << '\n';
            return std::expected<void, ErrorCode>{};
        })
        .or_else([](ErrorCode e) -> std::expected<void, ErrorCode>
        {
            std::cout << "Error: " << static_cast<int>(e) << '\n';
            return std::unexpected{e};
        })
    ;
}

The first thing to note is all of the sub-functions are much simpler. There is no need for any type aliases (well, I made one at the top to make the comparison simpler).

But wow, take a look at main(). A little bit of ergonomics goes a long, long way, and very quickly so.

And the std::expected version is not just simpler, it is also significantly more efficient. That is because there are vectors and strings being copied all over the place in Result version… but not in the std::expected version.

And that’s just the tip of the iceberg. If you used std::error_code as the error type, and proper error enumerations, then you could effortlessly combine operations from multiple libraries, and ultimately get the correct error code (if any). Instead of just printing “error” and a number, you could print an exact and clear error message… even from libraries you haven’t seen before.

Efficiency

As I mentioned above, as written, Result forces you to make copies unnecessarily.

Let’s assume ValueType is a std::string. Well, now just by checking res.value(), I am making a copy of the string.

By contrast, std::expected will return an lvalue reference if at all possible. But if that will dangle, it will return a move… not a copy, a move. So no copies will ever be made (unless you do something silly, like make the expected type const (foreshadowing…).

Aside from all the copying, there is another efficiency issue, which is also a correctness problem. When Result is constructed with an error code it still (default) constructs an instance of ValueType.

This is essentially the structure of Result:

template<typename T>
struct Result
{
    T m_value;
    E m_error;
    // discriminator so you know which of the value or error is active
};

So Result always hold both a value and an error. Not either. Both. That is not logically correct.

On the other hand, this is essentially the structure of std::expected:

template<typename T, typename E>
struct expected
{
    union
    {
        T m_value;
        E m_error;
    };
    // discriminator so you know which of the value or error is active
};

Or, more clearly:

template<typename T, typename E>
struct expected
{
    std::variant<T, E> _content;
};

In other words, a std::expected<T, E> is basically a std::variant<T, E>. Which, crucially, means that if it holds an error, there is no value. And if it holds a value, there is no error. There is only one or the other.

The most important thing

std::expected, and every type like it, has one critically important feature that Result is lacking.

Consider the following code:

// Assume this function.
auto func() -> Result<int>;

auto x = func().value();

Now it’s obvious what anyone should expect to happen if func() succeeds. x should hold the value returned from func(). Right?

Okay, but… what happens if func() fails?

In every iteration of the expected monad I’ve ever seen, you would get an exception. std::expected throws std::bad_expected_access<E>. std::optional, which works as a poor man’s std::expected, throws std::bad_optional_access. Other implementations throw their own exceptions. This is indeed the whole point of .value(): checking if there is a value, and throwing if there isn’t. If you want unchecked access to the value, there is operator*.

And so what happens with Result?

x will hold a (copy of a) default-constructed ValueType.

In other words, if func() has failed, the error will be swallowed up, and you will be left holding a value that you think is the legitimate result of func()… but is in fact just an artifact of the internals of Result.

This seems wildly dangerous. The only “correct” way to work with a Result is to check .has_value() (or .has_error(), same diff), and only then to access .value(). But if you forget that .has_value() check, you get no compile or run time errors, not even an exception for your troubles. Instead the code appears to work, but you have a silent bug. This is the absolute worst case scenario.

If you want to additionally allow unchecked access to the value, that’s fine; if the user chooses that, then they accept the risks. But you must provide checked access. Without that, Result is impossible to use safely.

Code review

enum class ErrorCode : std::uint8_t
{
    OK,
    INVALID_PARAMETERS,
    DATA_UNAVAILABLE,
    UNKNOWN_ERROR
};

It is dodgy practice to specify the underlying type of an enumeration, unless you need it for ABI purposes, or for some other reason. You shouldn’t just make it a std::uint8_t just because you can.

(Also, keep in mind that fixed-size types like std::uint8_t aren’t necessarily portable, which is another reason it’s unwise to use it here.)

In addition, don’t use ALL_CAPS for enumerators.

Now, Result is hard-coded to use ErrorCode as its error type, which seems limited. Wouldn’t it make more sense to use something like std::error_code, and then make that error code enumeration a proper error code enumeration with std::is_error_code_enum? That would give you extensible support for error codes from multiple sources, including different libraries.

template <typename ValueType>
class Result
{
    const ValueType m_value;
    const ErrorCode m_error;
    const bool m_has_error;

public:
    static_assert(std::is_copy_constructible<ValueType>::value && std::is_copy_assignable<ValueType>::value, "ValueType must be both copy-constructible and copy-assignable");
    static_assert(std::is_move_constructible<ValueType>::value && std::is_move_assignable<ValueType>::value, "ValueType must be both move-constructible and move-assignable");

I am truly baffled by this.

First of all, you shouldn’t make data members const. That is an anti-pattern. All it accomplishes is crippling the type. Result is now neither copyable, nor movable, which makes it extremely difficult to work with. And for what gain?

But what makes it truly absurd is that you go to all that effort to verify that the value type is both copyable and movable… only to then turn around and make m_value neither of those things. I’m left scratching my head. Why must ValueType be copyable and movable? So that you can construct it? Okay, but then it also has to be default constructible, and you’re not testing for that.

    explicit Result(ErrorCode e) noexcept : m_value{}, m_error{e}, m_has_error{true}  {}

I see a lot of noexcept used in the code, and like the const abuse on the data members, I have to wonder if there was really any thought put into why each function is being marked noexcept, or if it was just added mechanically. noexcept means a function cannot possibly fail… it’s not something you just slap on functions mindlessly.

Let’s start with this constructor… is it really true that there is no possible way it can fail? Really really? Yeah?

Try using the following class as the value type, then think about that question again:

struct totally_noexcept_default_construction
{
    totally_noexcept_default_construction()
    {
        throw "whoops";
    }
};

And the next constructor isn’t even guaranteed not to throw with the types you used in the example:

    explicit Result(ValueType t) noexcept : m_value{t}, m_error{ErrorCode::OK}, m_has_error{false}  {}

When ValueType is std::vector<int>, and m_value is built by copy-constructing t, that means it is using std::vector<int>’s copy constructor. Which… unfortunately… can very easily throw.

You could improve things at least a little by moving rather than copying… which you should be doing anyway:

    explicit Result(ValueType t) noexcept
        : m_value{std::move(t)}
        , m_error{ErrorCode::OK}
        , m_has_error{false}
    {}

… but you still can’t honestly say this constructor is noexcept unless and until you make sure that even ValueType’s move constructor can’t throw.

But an even better idea than moving would be emplacement, which could be completely free—no copies or moves—in some circumstances. All you’d need to do is:

    template<typename... Args>
        requires std::constructible_from<ValueType, Args...>
    constexpr explicit Result(std::in_place_t, Args&&... args)
        noexcept(std::is_nothrow_constructible_v<ValueType, Args...>)
        : m_value(std::forward<Args>(args)...)
        , m_error{ErrorCode::OK}
        , m_has_error{false}
    {}

You should look at std::expected constructors to see other possibilities.

    [[nodiscard]] ValueType value() const noexcept { return m_value; }

Since both m_value and *this are const, this will necessarily involve copying the value. Copying is very often not noexcept.

Is it really necessary to copy the value every time you want to inspect it? Why can’t you return a reference to the value? (Even better, follow the pattern of std::expected, std::optional, and so many others, and return a reference only when it won’t dangle.)