Is it bad to use a ternary inside of an if condition?

Question

Let's suppose I have some sort of form that automatically performs a copy procedure when the user either focuses on a textbox, or highlights some text.

Let's suppose that the user has a preference of copying on focus or copying on highlight. By default, it copies on highlight... which leads us to this code

if(copyOnFocus ? hasFocus : isHighlighted) {
   copy();
}

I understand this code without any problem, but I have this nagging feeling that this is a bad idea because I've never seen a ternary inside of an if condition before.

Is this ok, or should it be refactored? If it needs to be refactored, what would you suggest?

Answer 1

bool shouldCopy = (copyOnFocus ? hasFocus : isHighlighted);
if (shouldCopy) copy();

There: no more nested conditions. Happiness ensues.

Answer 2

As you mention - I've actually never seen this before.

It is completely logical and would work just fine - but I imagine that not all programmers would find this easy to read and debug.

My general recommendation would be to refactor it and move the decision making process to a new function - like this:

bool shouldCopy() {
    if (copyOnFocus)
        return hasFocus;
    else
        return isHighlighted;
}

This also makes it a lot easier to set break points when debugging.

Answer 3

You can rewrite this using && and || if you want. Basically, you want the loop to execute if copyOnFocus and hasFocus are true, or if copyOnFocus is false and isHighlighted is true. This can be phrased as

if ((copyOnFocus && hasFocus) || (!copyOnFocus && isHighlighted))
    // ...

Some may find this more readable, although it doesn't seem much better to me. If you don't care about copyOnFocus being false as long as isHighlighted is true:

if ((copyOnFocus && hasFocus) || isHighlighted)
    // ...

This changes the semantics of the code, but may be valid depending on what you know about those variables.

Personally, I find this about as hard to read as the original idea: splitting it off into a function may indeed be best, but I wouldn't consider it very big of a deal.

Answer 4

No, not bad: your ternary inside if is good: concise yet readable. It's good to teach people who haven't seen it.

If a bug requires the details of your line to be breakpointed, temporarily substitute more verbose code, but remove it later.

Answer 5

Your intuitions are right in telling you that this is bad. Mixing two different types of conditional statements (if (condition) { ... } and condition ? ... : ...) makes it extremely confusing to read.

I recommend refactoring so it only uses one type of conditional expression. Something like this:

(copyOnFocus ? hasFocus : isHighlighted) ? copy() : doNothing();

Answer 6

There are many ways to skin the cat. I like both the answer by @mbanzon and the one by @Daniel. Since you posted only a small section of the code, I do not know whether these variables are local (most likely) or global, and what the rest of your function looks like. Here is one way (anyone who can add two number in their head should be able to do the DeMorgan's arithmetic as well):

...
if (copyOnFocus)
{
    if (!hasFocus)
    {
        return; // or continue/break if inside a loop
    }

    if (!isHighlighted)
    {
        return; // or continue/break if inside a loop
    }
}

copy();
...

Now, some people hate multiple return statements. I am not one of them; I think they are fine. Having a single bool shouldCopy() function is cleaner in that ... if I decided to use break or continue instead of return, then I would only need to do it in one place. The downside is: if the variables are local, then all three of them would need to be passed in to the shouldCopy(,,) function, and that is arguably too verbose (though the proponents of the functional programming would probably not mind).

Answer 7

I've never seen a ternary operator inside an if condition before — and I don't think there's anything wrong with it — but it did give me pause for a bit when I first saw it. I don't think any of the more complicated answers with intermediate variables or functions add any clarity.

You might consider writing it like this:

if (copyOnFocus
        ? hasFocus
        : isHighlighted)
{
    copy();
}

although I'm not really sure how much of an improvement that is, since the original was pretty clear to me in the first place (after a couple of seconds of initial surprise).

Answer 8

It entirely depends on the skill-level of all programmers that need to understand this.

I have no problem reading this. You don't either. But someone else might. If you are sure that all programmers in your org are comfortable with such things, there is no problem here at all.

However if you think someone might not understand this, you can add a comment. Don't explain, how this works - explain what it means! (Just like you did in your question).