10

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?

rianjs
  • 210
  • 1
  • 8
  • 7
    That line should be nuked from orbit. Twice for good measure. – Blrfl Sep 10 '14 at 03:29
  • 8
    A nit: A single pipe (vertical bar) is a bitwise-or in most languages, rather than a logical "or." It does not short-circuit if the left hand side is true. Since this conditional has side effects on the right side, it makes an especially big difference in the outcome. – Jonathan Eunice Sep 10 '14 at 04:07
  • So I agree it looks... "interesting". But so do goto's, and some folks still advocate for them under just the right circumstances. Can you post a more complete code snippet so we get a better context? With the use of the bitwise operator I'm quite curious... – J Trana Sep 10 '14 at 04:47
  • 2
    There are idioms used in various languages that would fall into this category, but because they are "well known" or "normal" do not cause problems. The line in the question doesn't seem to fall into the idiomatic use category so I'd avoid it. – Jaydee Sep 10 '14 at 08:38
  • 2
    @JonathanEunice, yes, some folks were confused about short-circuit evaluation. Not a typo on my part. – rianjs Sep 10 '14 at 12:18
  • 5
    Look at the bright side. You now know one more company where you DON'T want to interview. – John R. Strohm Sep 11 '14 at 00:36
  • The non-short circuit code is **deliberate** so that b++ always gets evaluated, and is the only thing that makes the code semi-tolerable. – user949300 Dec 19 '17 at 16:37

4 Answers4

23

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.

Telastyn
  • 108,850
  • 29
  • 239
  • 365
  • It was just a throwaway line in a method during class. It wasn't intended to do anything useful. – rianjs Sep 10 '14 at 03:18
  • 4
    Equivalently, C/C++ `while(v = *p++)` style scans through a zero-terminated array (e.g. C string) are pretty common and widely accepted. – Phil Miller Sep 10 '14 at 03:33
  • 3
    I've often considered loop conditionals of the form `while(c = input.read() != '\n')` to be reasonably idiomatic. –  Sep 10 '14 at 04:14
  • 1
    There may be a few common acceptable idioms, but clearly "being clever at the expense of clarity" falls in the NEVER camp. – Julia Hayward Sep 10 '14 at 10:45
  • I totally forgot about iter.MoveNext(). A completely reasonable case for a conditional having a side effect. Thanks! – rianjs Sep 10 '14 at 12:16
  • @BЈовић - no. See the last paragraph. – Telastyn Sep 11 '14 at 11:31
8

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).

Brendan
  • 3,895
  • 21
  • 21
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.

Kilian Foth
  • 107,706
  • 45
  • 295
  • 310
2

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) {...}

Franki
  • 21
  • 1