Is it ever OK for a conditional to have side effects? [closed]

Question

Closed. This question is opinion-based. It is not currently accepting answers.

---

Want to improve this question? Update the question so it can be answered with facts and citations by editing this post.

Closed 6 years ago.

Improve this question

I'm taking intermediate data structures course as a prereq for entry into the CS MS program at a University everyone in America has heard of. One line of code that was written in class caught my eye:

if (a > 33 | b++ < 54) {...}

This would not pass a code review at my workplace. If you wrote code like this in an interview, this would be a significant strike against you. (In addition to being a conditional with side effects, it's being clever at the expense of clarity.)

In fact, I've never seen a conditional with side effects, and Googling doesn't turn up much, either. Another student stayed behind after class to ask about it, too, so I'm not the only one who thought this was weird. But the professor was pretty adamant that this was acceptable code, and that he would write something like that at work. (His FT job is as a Principal SWE at a company you've all heard of.)

I cannot imagine a world in which this line of code would ever be acceptable, let alone desirable. Am I wrong? Is this OK? What about the more general case: conditionals with side effects? Are those ever OK?

Answer 1

There is one semi-conditional side effect I can think of that is okay: while(iter.MoveNext())

That said, I think this falls mostly into the "never is a really big qualifier" category. I can think of a few rare cases where I've seen it be acceptable, but in general this is vile and to be avoided.

I also cannot think of a scenario where that particular line would be acceptable, but I also cannot think of a scenario where that particular line would be useful, so it is hard to imagine the context it is in.

Answer 2

In my world, a read from memory may be considered a side effect (e.g. memory mapped IO).

Now, consider the following:

    while( ( *memory_mapped_device_status_register & READY_FLAG) == 0) {
       // Wait
    }

And compare to:

    status = *memory_mapped_device_status_register;
    while( ( status & READY_FLAG) == 0) {
        // Wait
        status = *memory_mapped_device_status_register;
    }

Did avoiding the side effect (the read) in the condition help readability; or did it just duplicate code and add clutter?

It's not OK for a condition to have side effects (if that makes the code less readable) and it is also OK for a condition to have side effects (if that makes the code more readable). The key factor is "readability". Everything else is rules created by fools in a misguided attempt to improve readability (while often having the opposite effect).

Answer 3

As always with such questions, this is a matter of degree. If there were unequivocal proof that any side effect within an if expression always led to worse code, then it wouldn't be legal to create those expressions. Language designers may be ideosyncratic, fallible humans, but they're not that stupid.

That said, what are examples of justified side effects within an if? For instance, assume you're legally required to record all and any access to the property P of an entity E for audit purposes. (Imagine you're working in an uranium-enrichment plant, and there are very strict legal controls on what your code is allowed to do and how it's supposed to do it.) Then any if that checks that property will cause the side effect of the audit log being extended.

That's a pretty clear-cut cross-cutting concern, it doesn't affect your reasoning about the program state (very much), and you can implement it so that it's completely invisible and non-distracting when you review the line with the if (hidden away in the accessor, or even better via AOP). I'd say that's a pretty clear case of a side effect that's not a concern. Similar situations might be imagined when you just want to count branch executions for profiling purposes, etc.

The more these mitigating circumstances disappear, the stranger the construct will become. If a particular loop type (e.g. if((c = getc()) == 'x') { quit(); } is well-known and accepted by the language community, then it's much less of a problem than when you invent it spontaneously, etc. Your example line falls short of that standard, but I could imagine much, much more horrible ones that I won't even type, they're so horrible.

Answer 4

Though really smelly code, it has the advantage of being simpler (and maybe faster if you don't have a good optimizing compiler) than the equivalent if (a > 33 | b < 54) {b++; ...} else b++;

but of course it is possible to optimize it to the following (but watch out! this one has different behavior in case of overflow!): b++; if (a > 33 | b < 53) {...}