5

I've set up a ternary operator in place of a pile of if-else's, the final expression being nullptr in order to finish the loop, like so:

int menuSelect;

std::string operation="";

(menuSelect==1)? operation = "string1":
(menuSelect==2)? operation = "string2":
(menuSelect==3)? operation = "string3":
(menuSelect==4)? operation = "string4":
(menuSelect==5)? operation = "string5":
(menuSelect==6)? operation = "string6":nullptr;

std::cout<<operation;

Is this bad practice? It seems to work perfectly well, but I worry it may be illegal in some edge-case (or otherwise flatly stupid).

4
  • 8
    switch would be more idiomatic
    – Mat
    Commented Dec 30, 2023 at 6:25
  • @Mat: Sure, but I'd like to know if this breaks any conventions. Can nullptr not be converted in my case?
    – Hench
    Commented Dec 30, 2023 at 6:37
  • 3
    You shouldn't do that. A ternary is an expression. Every expression must have a type. While it might look like the type could be void, I'm pretty sure that expression actually has type std::string& due to the return value of the string assignment operator. That means the expression nullptr would be equivalent to std::string(nullptr). Depending on C++ version this would throw an exception when evaluated, or fail to compile. A safer expression would be std::string().
    – amon
    Commented Dec 30, 2023 at 6:56
  • @amon Ah, thanks so much :) I was confused about where the conversions actually occurred.
    – Hench
    Commented Dec 30, 2023 at 7:21

5 Answers 5

10

This is not a good idea because the ternary operator is intend to be used as an expression, and the recommended C++ core guideline is to avoid complicated expressions.

Here you only use only the expression's side effects (assignment operator) and this practice has no advantage over a simple if chain, which is easier to read. Moreover, the type of the ternary expression is determined according to complex rules which would cause your trailing nullptr to be converted to std::string if the selected value is not within the given range, with all the risks that this entails.

A better alternative of this construct would be to have only one assignment with the result of your composed ternary operator:

std::string operation =
  menuSelect==1 ? "string1":
  menuSelect==2 ? "string2":
  menuSelect==3 ? "string3":
  menuSelect==4 ? "string4":
  menuSelect==5 ? "string5":
  menuSelect==6 ? "string6":
  "";

It's still tricky because of the priority rules, if you'd use something more complex than a string literal. Cascading if are slightly more wordy but much less error-prone in this regard.

Another alternative if you really want to be compact and avoid to repeat yourself would be:

std::string operation = (menuSelect<1 || menuSelect>6) ? "" : 
      vector<string> { "string1", "string2", "string3", "string4", "string5", "string6"}[menuSelect-1];

Online demo.

(The last example is sheer provocation; in real code, I'd have initialised a vector in a separate statement instead of constructing an anonymous one, again, for readability purpose).

3
  • In real code, you'd use std::array rather than vector, of course. Commented Jan 5 at 16:13
  • @TobySpeight and static ;-) If the commands would not be constant and could change at every occurence, I'd go for the switch, in line with Kernighan's advice and to avoid unnecessary computations :-) .
    – Christophe
    Commented Jan 5 at 22:20
  • 1
    Oh, yes, absolutely. Switch for me, too (probably in a separate function, so each case can be a return statement). Commented Jan 6 at 8:31
11

In C++23 the code will not compile, as constructing a std::string from nullptr is explicitly disabled.

Before that, constructing a std::string from a null pointer (of type const CharT*) caused Undefined Behavior, and is thus most inadvisable.

Anyway, using a ternary expression as an if-else-statement is hardly clear, and certainly not idiomatic, thus should be avoided.

1
  • 2
    +1 for the c++23 hint. It's good news that compilers will flag the nullptr construction instead of letting it be a time bomb (maybe I should have more explicit than just raising it as a risk)
    – Christophe
    Commented Dec 30, 2023 at 20:26
5

Illegal? no. But it can provoke undefined behavior.

You're just looking for a default value when menuSelect is some unanticipated value. Any good reason operation can't be "undefined"?

When the unexpected happens you don't have to throw an exception. You just need a system that will gracefully deal with values that show our expectations were not met. It's why NaN exists.

Rather than null, often descriptive explanations of what was wrong with our expectations are best. In fact, it's why fractions, negative numbers, imaginary numbers, and transcendental numbers exist=.

1
  • "Any good reason operation can't be "undefined"?" - Very, very, very true. Why I hadn't considered that, I have no idea. I'm gonna go palm-slap my forehead for the next half-hour lol.
    – Hench
    Commented Jun 7 at 20:26
5

Is this bad practice?

Yes, it is terribly unreadable. Unless your code is actually performance sensitive and you have profiled it, found this code path is the hot path, and shown this construct is significantly faster than readable alternatives, don't do it.

3
  • Totally agree. A bounds check and array or error handled map would be my go to here. Commented Dec 30, 2023 at 11:05
  • @LoztInSpace Indeed, this is what I showed as alternative in my answer. It works well with literals, but the array could be more tricky if the operation would be build with more complex calculations, especially as all the computations would have to be made upfront. As explained, a simple if chain could do (it would even be one byte shorter than op's code despite it would give the impression of being more wordy), and a switch could be the better alternative if there would be many more alternatives, in view of the jump-table optimisation ;-)
    – Christophe
    Commented Dec 30, 2023 at 12:05
  • 3
    If fact, OP's code is not faster than a chained if, since the sequencing rules require to go through all the O(n) conditions one after the other. If nanoseconds would matter, the fastest alternative would be a simple switch, thanks to the jump-table optimisation, which is O(1) as the vector approach but without the overhead. Now it appears that modern optimiser recognises the pattern and generates jump tables for both approaches OP's code vs switch statement jmp [QWORD PTR .L4[0+rax*8]] line 28
    – Christophe
    Commented Dec 30, 2023 at 12:40
-2

One problem doing it this way instead of a switch statement: You would use an enum and not an integer, and when you do that in a switch statement the compiler would likely tell you if not all cases are covered.

BTW. operator= “string” with no space on the left but one on the right side is confusing. Same with the ?.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.