Member Avatar for Cameron_4

<!DOCTYPE html> <head> <title>Traffic Light Sequence</title>
</head>
<body>
<script>

function Light(){
if (document.getElementById("Img").src ="Stop-Amber.png"){ then
document.getElementById("Img").src = "Stop.png" }
else if (document.getElementById("Img").src ="Go.png"){ then
document.getElementById("Img").src = "Stop-Amber.png" }
else if(document.getElementById("Img").src ="Amber.png"){ then
document.getElementById("Img").src = "Go.png" }
else if (document.getElementById("Img").src ="Stop.png") { then
document.getElementById("Img").src = "Amber.png"}}
</script>
<button type="button" onclick="Light()"> Change Image!</button> <img src="stop.png" name="Img" width="224" height="395" id="Img" style= "width:304px;height:500px; <onclick="light()">
</body>
</html>

Edited by Cameron_4
  • 3 Contributors
  • 5 Replies
  • 702 Views
  • 2 Days Discussion Span
  • Latest Post Latest Post by Cameron_4
Member Avatar for Traevel

Hi,

In JavaScript a single = is used to assign a value and a double == is used to check a value. Also the term then can be removed entirely. You also have an error in the image tag itself, style= "width:304px;height:500px; <onclick="light()"> should just be style= "width:304px;height:500px;">. You already have the onclick attribute. Lastly, the original image has stop.png as a source but you check for Stop.png in your function.

If I change your JavaScript to the following (note the == when checking and the = when assigning)

function light() {
  if (document.getElementById("Img").src == "Stop-Amber.png") {
    document.getElementById("Img").src = "Stop.png";
  } else if (document.getElementById("Img").src == "Go.png") {
    document.getElementById("Img").src = "Stop-Amber.png";
  } else if (document.getElementById("Img").src == "Amber.png") {
    document.getElementById("Img").src = "Go.png";
  } else if (document.getElementById("Img").src == "Stop.png") {
    document.getElementById("Img").src = "Amber.png";
  }
}

the sequence I get is

  • Stop
  • Amber
  • Go
  • Stop-Amber
  • Stop
  • etc.

Also, you might have to change the img url's to their full paths. I tried this in a JSFiddle and had to change all the image paths to https://fiddle.jshell.net/_display/Amber.png for it to work.

Considering this is probably a homework assignment, here are some general pointers.

  • Function names should begin with a lowercase letter, following camelCase.
  • Even though JavaScript has automatic semi-colon insertion (leading to both hilarious and frustrating problems) you should add them to let the parser know where your statement ends.
  • Giving an image the id Img will work, but it's not very useful when you have several. Try to come up with descriptive id's and names so that you'll be able to recognize them in larger projects.
  • Even though a <script> tag in a <body> has its uses (mostly for speed and loading reasons), remember that the official place for scripts is in the <head> tag. Since this one is called by an element there's no reason for it to be in the body; you can't click on an element before it's finished loading.
  • You use both the width/height property of the image and the style property to set the image dimensions and both have different values. Pick one or the other.
  • As an improvement, instead of making calls to document.getElementById("Img") on numerous occasions you can instead store it in a variable. Every time you call that function it will traverse the DOM (Document Object Model). That won't be noticeable in smaller projects, but the larger the model the more you'll notice the effects of all the unnecessary calls.

So for instance something like:

   function light() {
        var image = document.getElementById("Img");
        if (image.src == "Stop-Amber.png") {
            image.src = "Stop.png";
        } 
        // etc.
   }
Edited by Traevel because: Typo
commented: Excellent example of what Daniweb is all about. Hats off to you. +15
commented: +1 +14
Member Avatar for Cameron_4 
<!DOCTYPE html> 
<head> 
<title>Traffic Light Sequence</title>
<script>

function light(){
var img = document.getElementById("Traffic");
if (img.src == "stop-amber.png"){ 
img.src = "stop.png" ;
}
else if (img.src == "go.png"){ 
img.src = "stop-amber.png";
}
else if(img.src == "amber.png"){
img.src = "go.png";
}
else if (img.src == "stop.png"){ 
img.src = "amber.png";
}}

</script>
</head>
<body>
<button type="button" onclick="Light()"> Change Image!</button> 
<img src="stop.png" name="Traffic" id="Traffic" style= "width:304px;height:500px;">
</body>
</html>

okay so I made the suggested changes, I appreciate the help btw, and i'm having issues with the traffic lights not changing at all now.

Edited by Cameron_4 because: edit to code
Member Avatar for diafol

Thought I'd add an observation. Complicated conditionals (if /elseif...) based on real filenames are a pain. What if you change the filename / location of one of your images? Thankfully we can use an agnostic dataset items, and store things like filenames in an array. There are many ways to skin a cat, but perhaps this will show what I mean:

<img id="traffic" src="" data-counter="" />
<button onclick="changeLights();">Ping</button>
<script>
    //Four-phase light sequence in UK:
    var lights = [
        'http://www.drivingtesttips.biz/images/traffic-light-red.jpg',
        'http://www.drivingtesttips.biz/images/traffic-lights-red-amber.jpg',
        'http://www.drivingtesttips.biz/images/traffic-lights-green.jpg',
        'http://www.drivingtesttips.biz/images/traffic-lights-amber.jpg'
    ];

    function changeLights( init ) {

        var el = document.getElementById("traffic")
        if( init ){
            var newItem = 0;
        } else {
            var currentIndex = parseInt(el.dataset.counter);
            var lastIndex = lights.length - 1;
            var newItem = (currentIndex == lastIndex) ? 0 : currentIndex + 1;
        }
        el.dataset.counter = newItem;
        el.src = lights[newItem];
    }

    //Set up the img tag with an initial image / counter
    changeLights(true);
</script>

As traevel suggests - use a variable to store the DOM object ('el' here) you save time - in more ways than one.
Note the empty img tag - that's OK as js will populate it on initialisation (runs script on page load) - the init parameter set to true.

The data-counter attribute stores the current array index for the filenames. Array (lights) starts on index = 0.
The else branch of the conditional gets the data-counter value and turns it into an integer and checks whether it should be reset to 0 (stop) or incremented to the next index in the array.

//Edit - jsfiddle here (slight diffs to above): https://jsfiddle.net/rkcqgwuo/

Edited by diafol
commented: Without a doubt an improvement! I wouldn't have thought about using data-counter, nice. +7
Member Avatar for Traevel 
<button type="button" onclick="Light()"> Change Image!</button>

You're calling Light() but the method is now light(). Also you might have to change all the image url's to their full path (i.e. http://localhost/myproject/img/start.png). If they are local files you could use the image url's diafol posted. Be sure to read his post, that's the way to do it properly.

For instance, where I live the lights don't turn amber when you can start driving again (only when you're about to stop). With his solution you could adjust the code to my situation simply by removing the corresponding image from the array.

Member Avatar for Cameron_4

Thankyou for the help!! Changing the urls to the online images and the error on calling the function got it up and running! Thanks.

Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.