Conditional operator as a statement

SimonSchroeder

@JonB said in Conditional operator as a statement:

You have no problem with e.g. variable = b ? x() : y();, do you? Only with using it as a statement, b ? x() : y();, right?

You are correct, I don't have a problem with variable assignment. (I tried to clarify this by the examples I gave.) Still, I would say the general rule is to not use them. You should only use them when you really need them. Sometimes there is no other language feature that could achieve the same thing, and then it is totally fine. Just don't be too smart about it.

(I'm personally a heavy user of ?:. I have just accidentially found this 'beauty' in my source:

std::ifstream in_winter(type == MIN ? "stats-" + std::to_string(year) + "-winter-min.csv"
                      : type == MAX ? "stats-" + std::to_string(year) + "-winter-max.csv"
                                    : "stats-" + std::to_string(year) + "-winter-mean.csv");

MIN and MAX are enum values.)

J.Hilk

@SimonSchroeder said in Conditional operator as a statement:

(I'm personally a heavy user of ?:. I have just accidentially found this 'beauty' in my source:

std::ifstream in_winter(type == MIN ? "stats-" + std::to_string(year) + "-winter-min.csv"
: type == MAX ? "stats-" + std::to_string(year) + "-winter-max.csv"
: "stats-" + std::to_string(year) + "-winter-mean.csv");
MIN and MAX are enum values.)

alt text

JonB

@SimonSchroeder

std::ifstream in_winter(type == MIN ? "stats-" + std::to_string(year) + "-winter-min.csv"
                      : type == MAX ? "stats-" + std::to_string(year) + "-winter-max.csv"
                                    : "stats-" + std::to_string(year) + "-winter-mean.csv");

I think it's fine you chose ? : here. if else would have been much longer. But just for the record I have a thing about about factoring and not repeating. I would probably have written yours as something like:

var amount = (type == MIN) ? "min" : (type == MAX) ? "max" : "mean";
std::ifstream in_winter("stats-" + std::to_string(year) + "-winter-" + amount + ".csv");

:) To me this makes it clear that the "test" is simply for the word min/max/mean and everything else is the same. And the lines are not too long!

J.Hilk

@JonB I would have gone with a lambda function and a full switch case approach. We're dealing with enums after all, and it screams at me: "THIS WILL EXPAND TO MORE FILES!"

JonB

@J.Hilk said in Conditional operator as a statement:

lambda function and a full switch case approach

Nah....!! KISS!! :)

And for the record I wouldn't use a switch statement to return a simple value where there are only 2 explicit case and a default. Why write a multiline essay to pick between a couple of literal string values? Of course it's only IMHO, and each to their own....

J.Hilk

@JonB I value readability over simplicity for simplicity’s sake..

JonB

@J.Hilk I agree.
And personally I find a couple of ? :s, in one line, as more readable than a multiple line switch statement and a lambda. Each to their own :)

J.Hilk

@JonB

auto winter_stats_filename = [](int year, Type type) -> std::string {
    const char* suffix = "mean"; // Fallback
    switch (type) {
        case Type::MIN:  suffix = "min";  break;
        case Type::MAX:  suffix = "max";  break;
        case Type::MEAN: suffix = "mean"; break;
        default:
            assert(false && "winter_stats_filename: unexpected Type value");
            break;
    }
    return "stats-" + std::to_string(year) + "-winter-" + std::string(suffix) + ".csv";
};

std::ifstream in_winter(winter_stats_filename(year, type));

I agree, sometimes I have the correct opinion and sometime the others have to wrong opinion :P

JonB

@J.Hilk
To pick whether you want min, max or mean in a string I have to read through 10 lines of your code (and check a lambda for sanity to add to it). Madness! :)

I can grasp

var amount = (type == MIN) ? "min" : (type == MAX) ? "max" : "mean";

at a single glance.

(Yes, I know you have added enum-range checking in your code which adds a few lines compared to mine.)

J.Hilk

@JonB I'm sorry but very time I see chained ternary operators, I get an Undertale rash.

JonB

@J.Hilk Fair enough! But what is "an Undertale rash", Google talks about it in some game but that's it?

J.Hilk

@JonB Undertale is a game that’s kind of infamous among programmers for having huge, messy if-else chains in its code.
So when I say “Undertale rash,” I mean I get an allergic reaction to code that looks like that

Chris Kawa

I really don't like lambdas used like this. Pack it in a function and you first have to go to the end of it to see what it's doing and then go back up to see how it is doing it. Have 2 or 3 of those in a function and the flow is completely ruined - you have to jump around and scroll multiple screens up and down to figure out what's going on.
I'm with @JonB on this - readability doesn't mean verbosity. Often fewer words express the intent better than paragraphs of syntax with sprinkles of what the code is actually doing.

JonB

@J.Hilk said in Conditional operator as a statement:

So when I say “Undertale rash,” I mean I get an allergic reaction to code that looks like that

Not knowing about the game or its code, I wondered if you intended "Undertail rash" and were thinking of a rash on your private back bits! :)

Kent-Dorfman

So which is more appropriate then? I know my choice.

Gas myGas(LOW_OCTANE);
if (engine.IsHighCompression()) { myGas = Gas(HIGH_OCTANE); }

or

Gas myGas;
if (engine.IsHighCompression()) { 
    myGas = Gas(HIGH_OCTANE); 
} else {
    myGas = Gas(LOW_OCTANE);
}

or

Gas myGas(engine.IsHighCompression() ? HIGH_OCTANE : LOW_OCTANE);

JonB

@Kent-Dorfman
The ? : one for me. If other people care I'm guessing they would be the same.

J.Hilk

@Kent-Dorfman

3 Simple, readable, no unnecessary object construction or copy/assignment stuff. I would only question why it's not const.

Wouldn't change 1. If I encountered it in a code base, but it's not my style

2 urgh, there would need to be some more context in the function before I approved it in a code review :D

GrecKo

Pattern matching could help (latest proposal : P2688R5)

Example usage: https://godbolt.org/z/zqenMsEch

Kent-Dorfman

@JonB @J.Hilk
Yeah, my decreasing order of preference too is 3, 1, 2...I'm guilty of (1) quite freqently during freeform though to text coding. as far as constifying everything...I'm lazy, and it stifles creativity ;^)

SimonSchroeder

I would expect to see 2 from C++ beginners (and maybe even intermediates). The variable declaration does not look like it is doing something. (And it behaves differently than built-in types because those stay uninitialized (up until C++26) when declared like this. Does anybody know if [[indeterminate]] would avoid calling the constructor?)

I am working in a context where performance matters and I'm highly aware of unnecessary work. Option 1 would initialize once and maybe do a second assignment. Option 2 would initialize once and definitely assign afterwards. Option 3 only has a single initialization. I would go for option 3. (Even more pedantic: in option 2&3 it is not just an additional assignment, but also an additional call to the constructor (and destructor). At least use a setter in these cases.)

Now, I'm really curious if as of C++26 this would be as performant as option 3:

Gas myGas [[indeterminate]];
if (engine.IsHighCompression()) { 
    myGas = Gas(HIGH_OCTANE); 
} else {
    myGas = Gas(LOW_OCTANE);
}

If it is, we might rethink what the most readable and thus proper answer should be.