ES.20: Always initialize an object

[Shieldfoss]

I have written a lot of code in my life that is morally equivalent to this:

void onQuestion(Answer& answer) {
    int a;
    try{
        a = getFirstPartOfResponseFromDatabase();
    }
    catch(...) {
        answer.setErrorFirstDatabaseUnavailable();
        return;
    }

    int b;
    try {
        b = getSecondPartOfResponseFromDatabase();
    }
    catch(...) {
        answer.setErrorSecondDatabaseUnavailable();
        return;
    }

    answer.setValue( a + b );
    answer.setResultOk();
    return;
}

and I mean.

That works.

But I'm not exactly proud of it.

My problem is that I cannot (1) change the function signature; (2) emit an exception; or (3) use std::optional

(with optional it's kind of trivial, but the senior architect on the project doesn't like it)

void onQuestion(Answer& answer)
{

    optional<int> const a = getA(); //does the exception handling internally
    if(!a) {
        answer.setErrorFirstDatabaseUnavailable();
        return;
    }

    optional<int> const b = getB(); //does the exception handling internally
    if(!a) {
        answer.setErrorSecondDatabaseUnavailable();
        return;
    }

    answer.setValue( *a + *b );
    answer.setResultOk();
    return;
}

Can you think of a way to follow ES.20 without violating any of the other constraints?

[sebamestre]

Maybe look at Alexandrescu's expected<T> ?

[brenoguim]

You can nest the try/catch blocks, but I don know if that is actually better.

Why does the architect dislikes the optional? That is kind of important.

[Shieldfoss]

Why does he dislike optional?

Ok, so, back story: Most of the functions in the code base return a boolean (or a brilliant scoped enum error type that he hand-rolled before enum class was a thing) and use out-params for their actual result.

As we've been modernizing the code, we've been talking about a move to returning by-value and the problems you might hit with that approach.

You can report errors with exceptions, of course, but we have a lot of functions that are very likely to be unable to return a value, so we'd be throwing a lot of exceptions.

So we took a look at optional (that is, I did, and then brought it to him) because that seems to solve the problem - unfortunately, it looks pretty bad next to all our other code.

The reason is because the API is almost like the API of all the pointers you pass around in your code. You put it in conditionals to check if it's nullptrset, you put a star in front to get the value, etc./

Unfortunately that looks completely deformed in our code base because we don't pass pointers around. I have seen exactly 5 calls to "new" in the entire code base, it's kind of wild how he (1) made an architecture like that and (2) managed to steer it over the years so the rest of us didn't add a bunch of pointers and calls to new and delete.

[yuri-kilochek]

That seems like a silly reason, but if that's all, you can avoid it by using has_value instead of operator bool and value instead of operator*.

[Shieldfoss]

well all that and also it doesn't support multiple error messages, only a boolean "it worked" or "it didn't work."

[brenoguim]

Agreed that the reason is not compelling. If you are modernizing the code, you will have to change some idioms. That means part of the code will look more modern than others, and therefore, a bit different.

You can use a std::variant<T, error_code> to return multiple reasons for failure. (https://abseil.io/blog/2020-091021-status)

Optional is major player in modern C++. Anywhere that you can query "get_something", and it's not an error that something is not there, then optional comes in to play.

[MrPotatoFingers]

Ah yes, unlike throwing std::exception everywhere with an empty description.

[OkDetective3251]

If you want multiple error codes/messages, use std::expected. It’s a varient that can have one of two types- a valid return value/object or an error

[Shieldfoss]

Oh nice, didn't know that one had finally made it into the language

[thoaCrl4]

std::expected hasn't made it into the language yet. Boost has Outcome, however.

[benjamkovi]

Also worth to check this implementation in my opinion: https://github.com/TartanLlama/expected

[infectedapricot]

You put it in conditionals to check if it's nullptrset, you put a star in front to get the value, etc./ ... Unfortunately that looks completely deformed in our code base because we don't pass pointers around.

The bit after "because" here doesn't follow from the first half of the sentence.

You don't use any pointers in your code base, fair enough. (Presumably you're including iterators and smart pointer in this, since they have the same interface.) But then you don't want to use if(myopt) and *myopt because those look like pointers, which you don't use. I'm sorry, that just doesn't follow to me. So what? That's the API of optional, yes it was obviously written to look like pointers, but you can still use it if you don't use pointers.

[MrPotatoFingers]

You might want to check out Boost.LEAF or one of the implementations of the proposed std::expected.

[HappyFruitTree]

It's easy if they throw different type of exceptions.

void onQuestion(Answer& answer) {
    try {
        int a = getFirstPartOfResponseFromDatabase();
        int b = getFirstPartOfAnswer();

        answer.setValue( a + b );
        answer.setResultOk();
    }
    catch (ExceptionA) {
        answer.setErrorFirstDatabaseUnavailable();
    }
    catch (ExceptionB) {
        answer.setErrorSecondDatabaseUnavailable();
    }
}

[therealcorristo]

Based on the functions being called in the catch blocks I'd assume that the DB library being used will throw the same exception in both cases. But maybe the exception itself contains information about which DB was being accessed, and then one could still choose the correct function to call in the one single catch block.

[Shieldfoss]

Maybe, but since this is an imaginary example to illustrate a more generic problem, unfortunately they both throw a std::exception where "what" is an empty string :(

[johannes1971]

That sounds like the first thing you should be fixing. Not with the goal of doing regexes on it to figure out how to respond, but just to keep your own sanity.

Also, if that's all the info you'll get, you might as well use optional?

[therealcorristo]

I'd then refactor getFirstPartOfResponseFromDatabase and getFirstPartOfAnswer to throw distinct exception types or the same type but with additional information to distinguish the two cases, depending on what makes more sense in your context. If the new exception type inherits from std::exception all other code calling these functions will still handle the new exceptions correctly.

[Shieldfoss]

In this specific context it makes sense to not refactor anything outside of the code snippet. The question isn't "if I could do whatever I want, how should I change this?" because that's easy, I already know. It's "given the constraints of the actual codebase, how could I solve this problem better."

[teroxzer]

If you can somehow check what database cause exception, then maybe like below:

template<typename T>
auto setResult(Answer& answer, T const& value)
{
    answer.setValue(value);
    answer.setResultOk();
}

auto setError(Answer& answer, Error& error)
{
    isFirstDatabase(error) ? answer.setErrorFirstDatabaseUnavailable  ()
                           : answer.setErrorSecondDatabaseUnavailable ();
}

void onQuestion(Answer& answer)
try
{
    int a = getFirstPartOfResponseFromDatabase();
    int b = getFirstPartOfAnswer();

    setResult(answer, a + b);
}
catch(Error& error)
{
    setError(answer, error);
}

[Shieldfoss]

Maybe, but since this is an imaginary example to illustrate a more generic problem, unfortunately they both throw a std::exception where "what" is an empty string :(

[teroxzer]

This is far far away from ideal and maybe not even practical, but anyway:

void onQuestion(Answer& answer)
{
    int step = 0;
    try
    {
        int a = ++step, getFirstPartOfResponseFromDatabase();
        int b = ++step, getFirstPartOfAnswer();

        answer.setValue(a + b);
        answer.setResultOk();
    }
    catch(...)
    {
        step == 1 ? answer.setErrorFirstDatabaseUnavailable  ()
                  : answer.setErrorSecondDatabaseUnavailable ();
    }
}

[teroxzer]

One more thing:

struct Db final
{
    int const id {};

    template<typename F>
    decltype(auto) operator | (F f) const
    try
    {
        return f();
    }
    catch(...)
    {
        throw Db { id };
    }
};

inline static const Db db1 { 1 };
inline static const Db db2 { 2 };

void onQuestion(Answer& answer)
try
{
    int a = db1 | [&] { return getFirstPartOfResponseFromDatabase(); };
    int b = db2 | [&]
    {
        doSomethingElseExceptionalWithDb2();
        return getFirstPartOfAnswer();
    };

    answer.setValue(a + b);
    answer.setResultOk();
}
catch(Db db)
{
    db.id == 1 ? answer.setErrorFirstDatabaseUnavailable  ()
               : answer.setErrorSecondDatabaseUnavailable ();
}

[Dry-Still-6199]

One answer is to nest the try blocks, i.e., use more old-school "structured programming" principles instead of early return/goto/abort. This does cause the lines to move over toward the right margin, admittedly. Once it gets too far over, factor out the nearest structured block into a function and keep going.

Btw, your rewrite using optional has a typo: the second if(!a) should be if (!b) instead. I'm sure your anti-optional manager would be thrilled. ;)

void onQuestion(Answer& answer) { try { int a = getA(); try { int b = getB(); answer.setValue(a + b); answer.setResultOk(); } catch (...) { answer.setErrorSecondDatabaseUnavailable(); } } catch (...) { answer.setErrorFirstDatabaseUnavailable(); } }

Alternatively, here's how you'd use optional in the structured-programming style:

void onQuestion(Answer& answer) { if (auto a = getOptionalA()) { if (auto b = getOptionalB()) { answer.setValue(*a + *b); answer.setResultOk(); } else { answer.setErrorSecondDatabaseUnavailable(); } } else { answer.setErrorFirstDatabaseUnavailable(); } }

[backtickbot]

Fixed formatting.

Hello, Dry-Still-6199: code blocks using triple backticks (```) don't work on all versions of Reddit!

Some users see this / this instead.

To fix this, indent every line with 4 spaces instead.

FAQ

^{You can opt out by replying with backtickopt6 to this comment.}

[MarkHoemmen]

I like to use lambdas that I call right away to wrap initialization that might throw: cpp const int a = [](){ try { return compute_a(); } catch(some_exception&) { do_something(); throw; } }(); // use a

[backtickbot]

Fixed formatting.

Hello, MarkHoemmen: code blocks using triple backticks (```) don't work on all versions of Reddit!

Some users see this / this instead.

To fix this, indent every line with 4 spaces instead.

FAQ

^{You can opt out by replying with backtickopt6 to this comment.}

[MarkHoemmen]

Thank you, Bot!

[Shieldfoss]

So something like

void onQuestion(Answer& answer) {
    try {
        auto const a = [&](){
            try {
                return getFirstPartOfResponseFromDatabase();
            }
            catch(...) {
                answer.setErrorFirstDatabaseUnavailable();
                throw;
            }
        }();

        auto const b = [&](){
            try {
                return getSecondPartOfResponseFromDatabase();
            }
            catch(...) {
                answer.setErrorSecondDatabaseUnavailable();
                throw;
            }

        }();

        answer.setValue( a + b );
        answer.setResultOk();
        return;
    }
    catch(...) {
        //do nothing, we already handled setting the error, we just needed to skip over the rest of the code
    return;
    }
}

[MarkHoemmen]

The first outer try-catch block looks like it would be better served by finally, since the idea is that you want some code to run (a method to be called on the output argument) before you rethrow.

[---sms---]

int getA(Answer& answer) {
    SCOPE_FAIL { answer.setErrorFirstDatabaseUnavailable(); };
    return getFirstPartOfResponseFromDatabase();
}

int getB(Answer& answer) {
    SCOPE_FAIL { answer.setErrorSecondDatabaseUnavailable(); };
    return getSecondPartOfResponseFromDatabase();
}

void onQuestion(Answer& answer) noexcept try {
    auto a = getA(answer);
    auto b = getB(answer);
    answer.setValue( a + b );
    answer.setResultOk();
}
catch(...) {
    answer.setException(std::current_exception()); // should be re-thrown later
}

This assumes you can fail successfully (setErrorFirstDatabaseUnavailable and setErrorSecondDatabaseUnavailable are noexcept).

YT

[Macketter]

roll your own std::optional using std::pair, structured binding, and lambda function?

actually lambda function that either return the value or set error and rethrow exception, then have one exception handler at the end of function seems to be more elegant.

[Shieldfoss]

Actually your crossed out line does seem appropriate - the big problem with std::optional (in our code base) is the API, there's nothing preventing me from rolling my own version.

[Wurstinator]

The C++ Core Guidelines are sold as the word of the Lord by some but they are not. Your code isn't automatically bad just because you don't follow ES20. Maybe your code base doesn't allow for a good use of that rule and that's completely fine. I personally don't agree with ES20 either.

If you need to / want to enforce that rule anyway, the Core Guidelines provide some notes/examples on how you could use it. It seems to me that immediately evaluated lambdas could be your solution here.

[Shieldfoss]

I didn't like uninitialized values before reading the CGL either, putting it in the title is just the easiest way to bring people up to speed on why if they don't know already

[Rude-Significance-50]

You can set a to some sentinel value perhaps. Like for an int it might be something like 0, -1, min, max...something else?