(JAVASCRIPT) Not updating my background color?

Question

I'm trying to do a project for my CS assignment, but for some reason, it refuses to run the code correctly.

  shake();
  if (answers == 0 || 2 || 3 || 5) {
    setProperty("screen1", "background-color", "green");
  } else if (answers == 1 || 4) {
    setProperty("screen1", "background-color", "red");
  } else {
    
  }

I set already set 'answers' to var answers = 0 and the shake function basically just randomizes a number for answers. However, even if answers == 1 or answers == 4, it still shows the background screen being green, not red. Any help?

`answers == 0 || 2 || 3 || 5` -> `answers == 0 || answers == 2 || answers == 3 || answers == 5` and `answers == 1 || 4` -> `answers == 1 || answers == 4` — VLAZ, Mar 15 '21 at 22:00
That syntax isn't JavaScript, it's just wishful thinking. Hint: What does `0 || 1` evaluate to? Know your [operator precedence](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence) rules. This is a prime candidate for a `switch` statement. — tadman, Mar 15 '21 at 22:01
Relevant: [Check variable equality against a list of values](https://stackoverflow.com/q/4728144) — VLAZ, Mar 15 '21 at 22:02
@tadman I'd not convert to `switch`. Seems much more compact to just do `[0, 2, 3, 5].includes(answers)`. Having five lines of fallthroughs just to mark five values as equal is a bit excessive. — VLAZ, Mar 15 '21 at 22:03
@VLAZ Thank you, that was a really simple fix. appreciate it. — skyzer01, Mar 15 '21 at 22:05
@VLAZ That creates "garbage" in the form of a temporary array that has to be dealt with later. `switch` is always faster than a linear array scan, too. If using an array, might as well use `const` and a look-up table, like `const COLORS = [ 'green', 'red', 'green', 'green', 'red', 'green' ]` and then just `COLORS[answers]`. — tadman, Mar 15 '21 at 22:09
@tadman if an array of five items is really the only bottleneck of your application, you're probably fine. Besides, I thought it obvious that you can extract the array. Seemed like adding that details would be unnecessary garbage to my comment. Should I endeavour to point out things like that in the future, thus making larger and longer comments that take more time to read and more or less repeat information that should be self-evident? Because if I were to point out self-evident details in each of my comments I think that overall hurts the information I want to put through in said comments. — VLAZ, Mar 15 '21 at 22:22
@tadman in other words, I was trying to be concise. Do you believe the above comment is *better* than this one? — VLAZ, Mar 15 '21 at 22:23
@VLAZ Unnecessary snark. I'm mentioning the garbage problem because although this might not be performance critical code, it's a bad habit to get into, and additionally, it is *not obvious* to programmers who are at a stage where they think `x == y || z` is valid code. — tadman, Mar 15 '21 at 22:24
@tadman it *is* valid code. It's just not doing what's expected of it. I don't think extracting a variable is such a huge leap to take. `switch` statements tend to hurt readability more if you have a lot of cases in them. I think doing grouping with `case`s is far worse habit than using an array lookup that the compiler might even optimise away. If you start grouping 15 or more values as fallthroughs, the readability of the code starts to suffer. — VLAZ, Mar 15 '21 at 22:33
@VLAZ That's why I suggested both the array (look-up version) as an improvement on your idea and `switch`. I think you're taking way more umbrage here than is warranted. — tadman, Mar 15 '21 at 22:37

score 1 · Answer 1 · answered Mar 15 '21 at 22:28

Two solutions here worth exploring. One is to use a switch statement which is generally going to be the fastest:

function screenBg(answer) {
  switch (answer) {
    case 0:
    case 2:
    case 3:
    case 5:
      return 'green';
    case 1:
    case 4:
      return 'red';
}

// In action:
setProperty("screen1", "background-color", screenBg(answers));

It can be a little weird to see a fall-through style of switch, which is why a look-up table is a good alternative:

const SCREEN_BG = [
  'green', // 0
  'red',   // 1
  'green', // 2
  'green', // 3
  'red',   // 4
  'green'  // 5
];

setProperty("screen1", "background-color", SCREEN_BG[answers]);

Where here the correlation between index and colour should be pretty clear.

score 0 · Answer 2 · answered Mar 15 '21 at 22:22

do it like this

shake();
  if ([0,2,3,5].includes(answers)) {
    setProperty("screen1", "background-color", "green");
  } else if ([1,4].includes(answers)) {
    setProperty("screen1", "background-color", "red");
  } else {
    
  }

the way your testing your if statement is wrong, it should be like this:

if(answers === 0 || answers === 2 || ...)

score 0 · Answer 3 · answered Mar 15 '21 at 22:24

Your conditions are incorrect and code can be improved a little bit.

let color = [0, 2, 3, 5].includes(answers) 
    ? 'green' 
    : [1, 4].includes(answers) 
        ? 'red'
        : 'orange';
setProperty("screen1", "background-color", color);

If you prefer to use if-elseif-else

let color = "orange"; // this is for else block.
if ([0, 2, 3, 5].includes(answers)) color = "red";
if ([1, 4].includes(answers)) color = "green";

If "answers" is an array, use the condition like this:

[0, 2, 3, 5].some((x) => answers.includes(x))

(JAVASCRIPT) Not updating my background color?

3 Answers3