Im trying to show the traffic lights in a loop

Question

I am trying to build this: Build a traffic light where the lights switch from green to yellow to red after predetermined intervals and loop indefinitely. Each light should be lit for the following durations:

Red light: 4000ms Yellow light: 500ms Green light: 3000ms

Please review and suggest if this is the right solution

 let i=0;
            const classNames = ['red','yellow','green'];
            const timeouts = [4000,500,3000]
            const interval = setInterval(() => {
               

                
                const div =  document.getElementById(`${classNames[i]}`)
    
                if(i>=0) {
                    const prevDiv =(i===0)?  document.getElementById(`${classNames[2]}`) : document.getElementById(`${classNames[i-1]}`)
                    prevDiv.className=''
                }
                div.className=classNames[i];
                
             
                if(i<2) {
                    i++;
                }else {
                    i=0;
                }


            },timeouts[i])

     .red {
            background-color: red;
        }
        .yellow{ 
            background-color: yellow;
        }
        .green {
            background-color: green;
        }

 <div id="red">Red</div>
            <div id="yellow">Yellow</div>
            <div id="green">Green</div>

Answer 1

Is your question on topic?

Your code does not work as expected. However the rules for asking at code review states that code "...works correctly (to the best of your knowledge)" and timing bugs are very difficult for humans to detect. I assume that you were unaware of the timing issue, and potencial hidden bugs.

Review

There are several problems with your solution.

Execution flow and order

You use the variable i to select the interval for the call to setInterval callback. In the callback you change i, however the change to i is never used to start another interval, so the timer callback continues to use the interval at i = 0 resulting in each light being on for 4000ms

Nothing happens if you don't execute the code. IE changing i does not affect previous use of i.

Luck can cover bugs

Often code can have bugs that do not occur due to pure luck.

In this case you use...

document.getElementById(`${classNames[i]}`)

...to get elements.

The luck is that the element id is the same as the element class name. If the class name or id was to change the bug would manafest and most likely throw an error.

You should have used querySelector(className[i]) which would locate the first element with the class name className[i]

setInterval is evil

The timing function setInterval starts an ongoing timed callback. The callback is called as close as possible after the interval time.

It can not be called while other code is running.

It is affected by the page visibility and the OS state.

If you forget to store the handle, or the variable holding the handle is lost you can not stop the timer. Thus setInterval is the only way to create a memory leak in JS. For this reason you should never ever use setInterval.

Use setTimeout

setTimeout is similar to setInterval however it only calls the callback once.

It can not cause a memory leak.

To repeat the callback one must call setTimeout again. This lets you recalculate the time till the next event in the case that the current event is late.

Style points.

• Avoid repeating DOM queries. Store DOM elements in variable.

• There is no need to make string from strings. Eg

  document.getElementById(`${classNames[i]}`)
  // same as
  document.getElementById(classNames[i]); // classNames[i] is a string
  

  Note I ignored the unlucky bug. :(

• It is a bad habit declaring variables in the global scope. Use IIFE (Immediately Invoked Function Expression) to keep the global scope clean.

• While learning JS it is best to run your code in strict mode. To do this add the directive "use strict" to the first line of code in your JS script.

  Note once you have gained experience you will know why you always use strict mode.

Rewrite

The rewrite uses

• setTimeout to time events
• objects via factory function light to define each light
• the array lights to store all the lights
• elements are stored rather than queried from the DOM each time they are needed
• the function nextLight is used to turn off the current light, turn on the next light, and setup the timeout for the next light
• uses remainder operator % to cycle light index. EG lights.currentIdx = (lights.currentIdx + 1) % lights.length
• performance.now() to get a time in milliseconds (used to calculate when to call for the next light)

Note The timing is set from the first call, if the lights fall behind (due to page visibility) they will cycle at increased speed (interval of 0) to catch up.

Note Element ids are unique to the page

Note lights.time - performance.now() may be negative. This is ignored by setTimeout which has the shortest timeout of > 0ms

Note I changed the timing because 4000ms is way to long for me to wait on red

"use strict";
(()=>{
  const light = (className, interval, element) => ({className, interval, element});
  const lights = [
      light("red",   2000, redEl),
      light("yellow", 500, yellowEl),
      light("green", 1000, greenEl),
  ];
  lights.currentIdx = lights.length - 1;
  lights.time = performance.now(); // ms since page load
  nextLight();

  function nextLight() {
    var light = lights[lights.currentIdx];
    light.element.classList.remove(light.className);

    // Next light
    lights.currentIdx = (lights.currentIdx + 1) % lights.length;
    light = lights[lights.currentIdx];
    light.element.classList.add(light.className);
    lights.time += light.interval;
    setTimeout(nextLight, lights.time - performance.now());
  }
})();

.red    { background-color: red; }
.yellow { background-color: yellow; }
.green  { background-color: green; }

<div id="redEl"   >Red</div>
<div id="yellowEl">Yellow</div>
<div id="greenEl" >Green</div>