Is it wrong to use this function, which changes the material inside the condition, making the condition dependent on the order?

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 increases the value of the variable and returns something. If I use such a function in an if that contains a variable that it increments, the condition will be order dependent.

Is this good or bad practice for this and why?

Answer 1

The conditions depend on the order, regardless of whether you change the variables used in the state or not. The two if statements you used as an example are different and will differ from whether you use myFunction () or not. They are equivalent:

 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 wrong practice in your code is not the fact that you change the value of the operands of the condition inside the condition, but the fact that your application state is displayed and controlled inside a function that does not even accept this state change as a parameter. We usually try to isolate functions from code outside their scope and use their return value to affect the rest of the code. Global variables are a bad idea in 90% of cases, and as your code base gets bigger and bigger, they tend to create problems that are hard to track, debug, and solve.

Answer 2

This is bad practice for the following reasons:

• Code is much less readable than well-built code. This is very important if the code is subsequently reviewed by a third party.

• If myfunction is changed later, the code flow will be completely unpredictable and may require significant documentation updates.

• Small and simple changes can have drastic consequences for code execution.

• He looks like an amateur.

Answer 3

If you must ask, this is hardly a good practice. Yes, this is bad practice precisely for the reason you talked about: changing the order of the operands of a logical operation should not affect the result, and therefore side effects in conditions should usually be avoided. Especially when they are hidden in a function.

Whether the function is pure (only reads the state and executes some logic) or whether it changes the state should be obvious from its name. You have several options for fixing this code:

• put the function call before if :

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

• make the mutation explicit inside if :

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

• puts the logic inside the called function:

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

Answer 4

MyFunction violates the principle called Report, do not ask .

MyFunction changes the state of something that makes it a team. If MyFunction succeeds or somehow does not increase the value of a , it should not return true or false. He was given a task, and she should either try to succeed, or if she finds that work is not possible at the moment, she should rule out an exception.

The if clause predicate uses MyFunction as the query.

Generally speaking, requests should not show side effects (i.e., not change things that can be observed). A good query can be considered as a calculation in the same case for the same inputs, it should give the same outputs (sometimes described as "idempotent").

It is also important to know that these are recommendations that will help you and others understand the code. Code that may cause confusion will be. Code confusion is a hatchery for mistakes.

There are good templates, such as the Trier-Doer template, which can be used as your sample code, but everyone who reads it should understand what is happening, although the names and structure.

Answer 5

Actually, the code is more than one bad practice:

 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!") } 

• Mutates a variable in another area. This may or may not be a problem, but usually it is.

• Calls a function inside the conditions of an if . This does not cause problems in itself, but it is not very clean. It is better to assign the result of this function to a variable, possibly with a descriptive name. This will help those who read the code to understand what exactly you want to check inside this if . By the way, the function always returns true .

• Uses some magic numbers. Imagine someone else reading this code, and it is part of a large code base. What do these numbers mean? The best solution would be to replace them with well-known constants.

• If you want to support more posts, you need to add additional conditions. A better approach would be to make this configuration.

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(); 

• A class with an exact function that uses its own internal state, and not a bunch of functions that rely on some variable that can be located anywhere. Whether to use the class or not is a matter of choice. This can be done differently.

• Uses configuration. This means that you want to add more messages, you do not need to touch the code at all. For example, imagine the configuration comes from a database.

• As you can see, this changes the variable in the outer area of ​​the function, but in this case it does not cause any problems.

• Uses constants with a clear name. (well, maybe better, but bring this example with me).

Answer 6

A function that changes material. What is the world? This function should change the material and return different values ​​each time it is called.

Consider the dealCard function for a deck of playing cards. he uses cards 1-52. Each time it is called, it must return a different value.

 function dealCard() { ++a; return cards(a); } 

/ * we just assume that the array maps are shuffled * /

/ * for brevity, we assume that the deck is infinite and does not have a loop at 52 * /