Is it bad practice to change state inside of an if statement?

Question

I wrote some code that looks similar to the following:

String SKIP_FIRST = "foo";
String SKIP_SECOND = "foo/bar";

int skipFooBarIndex(String[] list){
    int index;
    if (list.length >= (index = 1) && list[0].equals(SKIP_FIRST) ||
        list.length >= (index = 2) && 
        (list[0] + "/" + list[1]).equals(SKIP_SECOND)){
        return index;
    }

    return 0;
}

String[] myArray = "foo/bar/apples/peaches/cherries".split("/");
print(skipFooBarIndex(myArray);

This changes state inside of the if statement by assigning index. However, my coworkers disliked this very much.

Is this a harmful practice? Is there any reason to do it?

Answer 1

Yes. This clearly reduces readability. What's wrong with the following code?

int skipFooBarIndex(String[] list){
    if(list.length >= 1 && list[0].equals(SKIP_FIRST)) 
        return 1;
    if(list.length >= 2 && (list[0] + "/" + list[1]).equals(SKIP_SECOND))
        return 2;
    return 0;
}

It's much easier to understand. In general, having side effects in expressions is discouraged as you'll be relying on the order of evaluation of subexpressions.

Assuming you count it as "clever" code, it's good to always remember Brian Kernighan's quote:

Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.

Answer 2

Yes, side effects are hard to follow when reviewing code.

Regarding reasons to do it: No, there is no real reason to do it. I haven't yet stumbled upon an if statement that can't be rewritten without side effects without having any loss.

Answer 3

There's an excellent reason not to do it: it's makes your code really hard to understand and reason about.

Answer 4

Why is this bad, exactly? If you can't change something's state inside an if...what else could you possibly still do there?

Edit: Misread the question. I thought it was about changing state inside Y, not inside X:

if (X) { Y }

Answer 5

The problem is that the code would generate multiple-WTFs in a code review session. Anything that makes people go "wait, what?" has got to go.

It's sadly easy enough to create bugs even in easy-to-read code. No reason to make it even easier.

Answer 6

The only thing wrong with it is that it's unfamiliar and confusing to people who didn't write it, at least for a minute while they figure it out. I would probably write it like this to make it more readable:

if (list.length >= 1 && list[0].equals(SKIP_FIRST)) {
    return 1;
}

if (list.length >= 2 && (list[0] + "/" + list[1]).equals(SKIP_SECOND)) {
    return 2;
}

Answer 7

Well, I spent some time reading the above without realising what was going on. So I would definitely suggest that it's not ideal. I wouldn't really ever expect the if() statement itself to change state.

Answer 8

I wouldn't recommend an if condition having side-effects without a very good reason. For me, this particular example took several looks to figure out what was going on. There may be a case where it isn't so bad, although I certainly can't think of one.

Answer 9

Ideally, each piece of code should do one thing. Making it do more than one thing is potentially confusing, and confusing is exactly what you don't want in your code.

The code in the condition of an if statement is supposed to generate a boolean value. Tasking it with assigning a value is making it do two things, which is generally bad.

Moreover, people expect conditions to be just conditions, and they often glance over them when they're getting an impression of what the code is doing. They don't carefully parse everything until they decide they need to.

Stick that in code I'm reviewing and I'll flag it as a defect.

Answer 10

Borrowed from cppreference.com:

One important aspect of C++ that is related to operator precedence is the order of evaluation and the order of side effects in expressions. In some circumstances, the order in which things happen is not defined. For example, consider the following code:

float x = 1;
x = x / ++x;

The value of x is not guaranteed to be consistent across different compilers, because it is not clear whether the computer should evaluate the left or the right side of the division first. Depending on which side is evaluated first, x could take a different value.

Furthermore, while ++x evaluates to x+1, the side effect of actually storing that new value in x could happen at different times, resulting in different values for x.

The bottom line is that expressions like the one above are horribly ambiguous and should be avoided at all costs. When in doubt, break a single ambiguous expression into multiple expressions to ensure that the order of evaluation is correct.

Answer 11

...However, my coworkers disliked this very much...

Yes, it is. Not just because you can code it like that, you have to.

Remember that that piece of code will eventually have to be maintained by someone ( that someone may be your self in 8 months )

Changing the state inside the if, make is harder to read and understand ( mostly because it is non common )

Quoting Martin Fowler:

Any fool can write code that a computer can understand. Good programmers write code that humans can understand

Answer 12

In C it's fairly common to change state inside if statements. Generally speaking, I find that there are a few unwritten rules on where this is acceptable, for example:

• You are reading into a variable and checking the result:

  int a;
  ...
  if ((a = getchar()) == 'q') { ... }
  

• Incrementing a value and checking the result:

  int *a = (int *)0xdeadbeef;
  ...
  if (5 == *(a++)) { ... }
  

And when it is not acceptable:

• You are assigning a constant to a variable:

  int a;
  ...
  if (a = 5) { ... } // this is almost always unintentional
  

• Mixing and matching pre- and post-increment, and short-circuiting:

  int a = 0, b;
  ...
  if (b || a++) { ... }    // BAD!
  

For some reason the font for sections I'm trying to mark as code is not fixed-width on SO, but in a fixed width font there are situations where assignment inside if expressions is both sensible and clear.

Answer 13

You can also get ternary to avoid multiple returns:

int skipFooBarIndex(String[] list) {
    return (list.length > 0 && list[0].equals(SKIP_FIRST)) ? 1 :
       ((list.length > 1 && (list[0] + "/" + list[1]).equals(SKIP_SECOND)) ? 2 : 0);
}

Though this example is less readable.

Answer 14

Speaking as someone who does a lot of maintenance programming: if I came across this I would curse you, weep and then change it.

Code like this is a nightmare - it screams one of two things

1. I'm new here and I need help doing the right thing.
2. I think I am very clever because I have saved lines of code or I have fooled the compiler and made it quicker. Its not clever, its not optimal and its not funny

;)

Answer 15

Is this a harmful practice?

Absolutely yes. The code is hard to understand. It takes two or three reads for anyone but the author. Any code that is hard to understand and that can be rewritten in a simpler way that is easier to understand SHOULD be rewritten that way.

Your colleagues are absolutely right.

Is there any reason to do it?

The only possible reason for doing something like that is that you have extensively profiled the application and found this part of code to be a significant bottleneck. Then you have implemented the abomination above, rerun the profiler, and found that it REALLY improves the performance.