Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have create a functioning automated traffic light sequence using an array and if statements. It all work correctly but I am wondering if there is anything more I can do to improve my code without changing to structure or way it works, so without the use of dictionary's etc.

<!DOCTYPE html>
<head>
    <title> Traffic Light</title>   

    <style>
        .rainbow {
            background-image: -webkit-gradient( linear, left top, right top, color-stop(0, red), color-stop(0.1, yellow), color-stop(0.2, green));
            background-image: gradient( linear, left top, right top, color-stop(0, #f22), color-stop(0.15, #f2f), color-stop(0.3, #22f), color-stop(0.45, #2ff), color-stop(0.6, #2f2),color-stop(0.75, #2f2), color-stop(0.9, #ff2), color-stop(1, #f22) );
            color:transparent;
            -webkit-background-clip: text;
            background-clip: text;
        }
    </style>

</head>

<body background="street.gif">
    <h1 class="rainbow">Traffic Light</h1>

    <canvas id="myCanvas" width="200" height="300"
    style="border:1px solid #000000;">
    Your browser does not support the HTML5 canvas tag.
    </canvas>


    <script>   

        var c = document.getElementById("myCanvas");
        var ctx = c.getContext("2d");

        ctx.rect(0, 0, 200, 300);
        ctx.fillStyle = "grey";
        ctx.fill();

        var colours=["red", "yellow", "green", "black","red yellow"];
        var current=colours[0];

        function offlight() {
            ctx.beginPath();
            ctx.arc(95,50,40,10,12*Math.PI);
            ctx.fillStyle = "black";
            ctx.fill();
            ctx.stroke();
        }

        function offlight1() {
            ctx.beginPath();
            ctx.arc(95,150,40,10,12*Math.PI);
            ctx.fillStyle = "black";
            ctx.fill();
            ctx.stroke();
        }

        function offlight2() {
            ctx.beginPath();
            ctx.arc(95,250,40,10,12*Math.PI);
            ctx.fillStyle = "black";
            ctx.fill();
            ctx.stroke();
        }

        function drawLight1() {
            ctx.beginPath();
            ctx.arc(95,50,40,10,12*Math.PI);
            ctx.fillStyle = "red";
            ctx.fill();
            ctx.stroke();
        }

        function drawLight2() {
            ctx.beginPath();
            ctx.arc(95,150,40,10,12*Math.PI);
            ctx.fillStyle = "yellow";
            ctx.fill();
            ctx.stroke();
        }

        function drawLight3() {
            ctx.beginPath();
            ctx.arc(95,250,40,10,12*Math.PI);
            ctx.fillStyle = "green";
            ctx.fill();
            ctx.stroke();
        }

        function changelight(){

            if (current==colours[0]){
                drawLight1();
                offlight1();
                offlight2();
                current=colours[4]
            } else if (current==colours[4]){
                drawLight1();
                drawLight2();
                offlight2();
                current=colours[2]
            } else if (current==colours[2]) {
                offlight();
                offlight1();
                drawLight3();
                current=colours[3]
            } else if (current==colours[3]){
                offlight();
                drawLight2();
                offlight2();
                current=colours[0]
            }

        }
        setInterval(changelight,1000);

    </script>

    <br><br>
    <button onclick="changelight()">Click</button>

</body>

share|improve this question

migrated from stackoverflow.com Feb 15 at 21:01

This question came from our site for professional and enthusiast programmers.

  1. you don't ever use the strings in colors, but use them only as kind of id, what you are showing: an index would be sufficient, even the better solution.

  2. you don't use the order in colors but instead you jump manually; sub-optimal, especially since you don't have any logic in there but just run a simple loop

  3. And you repeat yourself excessively.

You have three lights, that are either on or off. To me this screams for the use of a bit-mask: The State is represented by an integer, and each bit of this integer represents one of the lights: it's either 1 or 0.

var c = document.getElementById("myCanvas");
var ctx = c.getContext("2d");
ctx.rect(0, 0, 200, 300);
ctx.fillStyle = "grey";
ctx.fill();

//an array of the states, the code loops through
var activeLights = [0b100, 0b110, 0b001, 0b000, 0b010];
//var activeLights = [4, 6, 1, 0, 2];   //if your browser doesn't get the binary-notation
var currentIndex = 0;

function drawLight(index, color){
    ctx.beginPath();
    ctx.arc(95,50+index*100,40,10,12*Math.PI);
    ctx.fillStyle = color;
    ctx.fill();
    ctx.stroke();
}

function changelight(){
    var state = activeLights[currentIndex];

    //check the bit and draw the respective light
    drawLight(0, state & 0b100? "red": "black");
    drawLight(1, state & 0b010? "yellow": "black");
    drawLight(2, state & 0b001? "green": "black");      
    //drawLight(0, state & 4? "red": "black");
    //drawLight(1, state & 2? "yellow": "black");
    //drawLight(2, state & 1? "green": "black");

    //the loop
    if(++currentIndex === activeLights.length) currentIndex = 0;
}

setInterval(changelight, 1000);
share|improve this answer

You should move your <h1> into the <body>.

What exactly are you looking for? File size (--> code reduction)? Speed optimization?

One simple thing would be using parameters to reduce code duplication:

var c = document.getElementById("myCanvas");
var ctx = c.getContext("2d");

ctx.rect(0, 0, 200, 300);
ctx.fillStyle = "grey";
ctx.fill();

var colours = ["red", "yellow", "green", "black", "red yellow"];
var current = colours[0];

function offlight(a1) {
    ctx.beginPath();
    ctx.arc(95, a1, 40, 10, 12 * Math.PI);
    ctx.fillStyle = "black";
    ctx.fill();
    ctx.stroke();
}

function drawLight(a1, fillParam) {
    ctx.beginPath();
    ctx.arc(95, a1, 40, 10, 12 * Math.PI);
    ctx.fillStyle = fillParam;
    ctx.fill();
    ctx.stroke();
}

function changelight() {
    if (current == colours[0]) {
        drawLight(50, "red");
        offlight(150);
        offlight(250);
        current = colours[4]
    } else if (current == colours[4]) {
        drawLight(50, "red");
        drawLight(150, "yellow");
        offlight(250);
        current = colours[2]
    } else if (current == colours[2]) {
        offlight(50);
        offlight(150);
        drawLight(250, "green");
        current = colours[3]
    } else if (current == colours[3]) {
        offlight(50);
        drawLight(150, "yellow");
        offlight(250);
        current = colours[0]
    }

}
setInterval(changelight, 1000);

Do you ever use colours[1] ("yellow")?

You could also remove the whitespace and shorten variable and function names. Also, think about moving your JS into a seperate file.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.