Is it bad practice to use a function that changes stuff inside a condition, making the condition order-dependent?

Question

var a = 1;

function myFunction() {
    ++a;
    return true;
}

// Alert pops up.
if (myFunction() && a === 2) {
    alert("Hello, world!");
}

// Alert does not pop up.
if (a === 3 && myFunction()) {
    alert("Hello, universe!");
}

https://jsfiddle.net/3oda22e4/6/

myFunction increments a variable and returns something. If I use a function like that in an if statement that contains the variable which it increments, the condition would be order-dependent.

Is it good or bad practice to do this, and why?

Answer 1

Conditions are order-dependent whether you change the variables used in the condition or not. The two if statements that you used as an example are different and will be different whether you use myFunction() or not. They are equivalent to:

if (myFunction()) {
   if (a === 2) {
     alert("Hello, world!")
   }
}

// Alert does not pop up.
if (a === 3) {
   if (myFunction()) {
     alert("Hello, universe!")
   }
}

In my opinion, the bad practice in your code is not the fact that you change the condition's operands value inside the condition, but the fact that your application state is exposed and manipulated inside a function that does not even accept this state changing variable as a parameter. We usually try to isolate the functions from the code outside their scope and use their return value to affect the rest of the code. Global variables are 90% of the time a bad idea and as your code base gets larger and larger they tend to create problems that are difficult to trace, debug and solve.

Answer 2

MyFunction violates a principle called Tell, Don't Ask.

MyFunction changes the state of something, thus making it a command. If MyFunction succeeds or somehow fails to increment a, it shouldn't return true or false. It was given a job and it must either try to succeed or if it finds that job is impossible at the moment, it should throw an exception.

In the predicate of an if statement, MyFunction is used as a query.

Generally speaking, queries should not exhibit side-effects (i.e. not changing things that can be observed). A good query can be treated like a calculation in that for the same inputs, it should produce the same outputs (sometimes described as being "idempotent").

It's also important to know that these are guidelines to help you and others reason about the code. Code that can cause confusion, will. Confusion about code is a hatchery for bugs.

There are good patterns like the Trier-Doer pattern which can be used like your code example, but everyone reading it must understand what's happening though names and structure.

Answer 3

If you have to ask, it's hardly a good practice. Yes, it's a bad practice for exactly the reason you mentioned: changing the order of operands of a logical operation should not affect the outcome, and therefore side effects in conditions should generally be avoided. Especially when they are hidden in a function.

Whether the function is pure (only reads state and does some logic) or whether it mutates state should be obvious from its name. You have several options to fix this code:

• put the function call before the if:

  function tryChangeA() {
      a++;
      return true;
  }
  
  var ok = tryChangeA();
  if (ok && a == 2) … // alternatively: if (a == 2 && ok)
  

• make the mutation explicit inside the if:

  function testSomething(val) {
      return true;
  }
  
  if (testSomething(++a) && a == 2) …
  

• put the logic inside the called function:

  function changeAndTest() {
      a++;
      return a == 2;
  }
  
  if (changeAndTest()) …
  

Answer 4

The code presents more then one bad practice actually:

var a = 1;
function myFunction() {
    ++a; // 1
    return true;
}

if (myFunction() && a === 2) { // 2, 3, 4
    alert("Hello, world!")
}

if (a === 3 && myFunction()) { // 2, 3, 4
    alert("Hello, universe!")
}

1. Mutates a variable in a different scope. This may or may not be a problem, but usually it is.

2. Calls a function inside an if statement condition. This do not cause problems in itself, but it's not really clean. It's a better practice to assign the result of that function to a variable, possibly with a descriptive name. This will help whoever reads the code to understand what exactly you want to check inside that if statement. By the way, the function always return true.

3. Uses some magic numbers. Imagine someone else reading that code, and it is part of a large codebase. What those numbers mean? A better solution would be to replace them with well named constants.

4. If you want to support more messages, you need to add more conditions. A better approach would be to make this configurable.

I would rewrite the code as follows:

const ALERT_CONDITIONS = { // 4
  WORLD_MENACE: 2,
  UNIVERSE_MENACE: 3,
};

const alertsList = [
  {
    message: 'Hello world',
    condition: ALERT_CONDITIONS.WORLD_MENACE,
  },
  {
    message: 'Hello universe',
    condition: ALERT_CONDITIONS.UNIVERSE_MENACE,
  },
];


class AlertManager {
  constructor(config, defaultMessage) {
    this.counter = 0; // 1
    this.config = config; // 2
    this.defaultMessage = defaultMessage;
  }

  incrementCounter() {
    this.counter++;
  }

  showAlert() {
    this.incrementCounter();
    let customMessageBroadcasted = false;
    this.config.forEach(entry => { //2
      if (entry.condition === this.counter) {
        console.log(entry.message);
        customMessageBroadcasted = true; // 3
      }
    });

    if (!customMessageBroadcasted) {
      console.log(this.defaultMessage)
    }
  }
}

const alertManager = new AlertManager(alertsList, 'Nothing to alert');
alertManager.showAlert();
alertManager.showAlert();
alertManager.showAlert();
alertManager.showAlert();

1. A class with a precise function, that use its own internal state, instead of a bunch of functions that rely on some variable that could be located anywhere. Whether to use a class or not, it's a matter of choice. It could be done in a different way.

2. Uses a configuration. That means that would you want to add more messages, you don't have to touch the code at all. For example, imagine that configuration coming from a database.

3. As you may notice, this mutates a variable in the outer scope of the function, but in this case it does not cause any issue.

4. Uses constants with a clear name. (well, it could be better, but bear with me given the example).