SitePoint Sponsor

User Tag List

Results 1 to 10 of 10
  1. #1
    SitePoint Member
    Join Date
    Apr 2008
    Posts
    8
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Question Code feedback and suggestion, please

    Hey there.

    I've written a simple image switching script based on several examples I found online. I wanted to write my own because I'm trying to learn js and because most of the prewritten scripts that I found didn't do exactly what I needed, were obtrusive, or were overkill.

    The script works great in all my testing, but I'm a little OCD so here I am.

    I'm looking for a little feedback. Mostly I just want to know if there's anything that jumps out as bad code, but other feedback is great too.

    I'm also looking for a suggestion as to how to make the script support multiple images. I think I could probably create arrays for the imageTarget variable and then loop through the array in the switcher function, but suggestions would be most welcome.

    Code is below.

    XHTML:
    Code:
    <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
    <html xmlns="http://www.w3.org/1999/xhtml">
    <head>
    	<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
    	<title>Switching Image</title>
    	<script type="text/javascript" src="imageSwitcher.js"></script>
    </head>
    <body>
    	<div id="wrapper">
    	
    	<img id="switchedImage" src="images/random185x185/blue.jpg" alt="box" width="185" height="185" />
    	
    	</div> <!-- End wrapper -->
    </body>
    </html>
    javascript:
    Code:
    // Populate this list with image file names
    var imageList = Array("cyan.jpg","blue.jpg","green.jpg","magenta.jpg","orange.jpg","yellow.jpg");
    // This sets the original value for the rotation loop. It's global. Don't change it.
    var count = 0;
    
    // This preloads the images to minimize delay when rotating.
    for (var i = 0; i < imageList.length; i++) {
    	imageObject = new Image() ;
    	// Set the quote-enclosed portion of the code below to reflect the path to the images you want to include in the rotation.
    	imageObject.src = ("images/random185x185/" + imageList[i]);
    }
    
    // This function performs the actual rotation of the image sources.
    function switcher() {
    	if (count > (imageList.length - 1))
    		count = 0;
    	// Set the quote-enclosed portion of the code below to reflect the id of the image to rotate.
    	var imageTarget = document.getElementById("switchedImage");
    	// Set the quote-enclosed portion of the code below to reflect the path to the images you want to include in the rotation.
    	imageTarget.setAttribute("src","images/random185x185/" + imageList[count]);
    	count++;
    	return count;
    }
    
    // This function sets the timing and triggers the rotation 
    function switchTimer() {
    	// Set the numeric value below to reflect, in milliseconds, the desired delay between rotation.
    	setInterval ("switcher()", 3500);
    }
    
    window.onload = switchTimer;
    Thanks in advance for any help/guidance.

  2. #2
    SitePoint Guru
    Join Date
    Apr 2006
    Posts
    802
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    var imageList= [];
    var A=["cyan.jpg","blue.jpg","green.jpg","magenta.jpg","orange.jpg","yellow.jpg"];
    var count= 0;

    //Your preloader only preloads the last image-
    //something like this will preload all 5 images:

    Code:
    while(A.length){	
    	imageList[count]= new Image() ;
    	imageList[count].src= "images/random185x185/" + A.shift();	
    	count++;
    }
    //this will simplify the switcher code-

    Code:
    function switcher(){
    	var url= imageList[count++] || imageList[0];
    	var imageTarget= document.getElementById("switchedImage");
    	imageTarget.setAttribute("src",url.src);
    	if(count<10) setTimeout('switcher()',3500);
    }
    // You should have a way to pause or stop the animation-
    //this does it by changing the value of count when the image is clicked

    Code:
    window.onload= function(){
    	document.getElementById("switchedImage").onclick=function(){
    		if(count<10) count+= 10;
    		else{
    			count-= 10;
    			switcher();
    		}
    	}
    	switcher();
    }
    Last edited by mrhoo; Apr 13, 2008 at 10:53.

  3. #3
    SitePoint Member
    Join Date
    Apr 2008
    Posts
    8
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Thanks mrhoo

    Thanks a lot for the suggestions:

    I much prefer your method for preloading but I couldn't get the rest of your code to work.

    The switcher function you suggest had no way of resetting the count variable when it exceeded the length of the imageList array.

    I also had difficulty when using setTimeout within the switcher function.
    I kept getting null errors, but it works great when I call switcher from another function. That also solved the count problem.

    My code:

    Code:
    // Create an empty array to hold the images
    var imageList = [];
    // Populate this array with image file names
    var A = ["cyan.jpg","blue.jpg","green.jpg","magenta.jpg","orange.jpg","yellow.jpg"];
    // This sets the original value for the rotation loop. It's global.
    var count = 0;
    
    // This preloads the images to minimize delay when rotating.
    // It also populates the empty imageList array with the path and filenames
    while (A.length) {
    	imageList[count] = new Image();
    	// Set the quote-enclosed portion of the code below to reflect the path to the images you want to include in the rotation.
    	imageList[count].src = ("images/random185x185/" + A.shift());
    	count++;
    }
    
    count = 0;
    
    // This function performs the actual rotation of the image sources.
    function switcher() {
    
    	var url = imageList[count].getAttribute("src");
    	var imageTarget = document.getElementById("switchedImage");
    	imageTarget.setAttribute("src", url);
    	count++;
    	if (count > (imageList.length - 1))
    		count = 0;
    		return count;
    }
    
    // This function sets the timing and triggers the rotation 
    function switchTimer() {
    	// Set the numeric value below to reflect, in milliseconds, the desired delay between rotation.
    	setInterval ("switcher()", 500);
    }
    
    window.onload = switchTimer();
    If my thinking is backwards on any of this, please let me know.

    Matt

  4. #4
    SitePoint Member
    Join Date
    Apr 2008
    Posts
    8
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Another question

    Why do I get a null error when using the code below?

    xhtml:
    Code:
    <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
    <html xmlns="http://www.w3.org/1999/xhtml">
    <head>
    	<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
    	<title>Switching Image</title>
    	<script type="text/javascript" src="imageSwitcher4.js"></script>
    </head>
    <body>
    	<div id="wrapper">
    	
    	<img id="switchedImage" src="images/random185x185/blue.jpg" alt="box" width="185" height="185" />
    	</div> <!-- End wrapper -->
    </body>
    </html>
    imageSwitcher4.js:
    Code:
    // Create an empty array to hold the images
    var imageList = [];
    // Populate this array with image file names
    var A = ["cyan.jpg","blue.jpg","green.jpg","magenta.jpg","orange.jpg","yellow.jpg"];
    // This sets the original value for the rotation loop. It's global.
    var count = 0;
    
    // This preloads the images to minimize delay when rotating.
    // It also populates the empty imageList array with the path and filenames
    while (A.length) {
    	imageList[count] = new Image();
    	// Set the quote-enclosed portion of the code below to reflect the path to the images you want to include in the rotation.
    	imageList[count].src = ("images/random185x185/" + A.shift());
    	count++;
    }
    
    count = 0;
    
    // This function performs the actual rotation of the image sources.
    function switcher() {
    
    	var url = imageList[count].getAttribute("src");
    	var imageTarget = document.getElementById("switchedImage");
    	imageTarget.setAttribute("src", url);
    	count++;
    	if (count > (imageList.length - 1))
    		count = 0;
    		return count;
    }
    
    // This function sets the timing and triggers the rotation 
    function switchTimer() {
    	var imageTarget = document.getElementById("switchedImage");
    	alert (imageTarget.getAttribute("src"));
    	// Set the numeric value below to reflect, in milliseconds, the desired delay between rotation.
    	setInterval ("switcher()", 500);
    }
    
    window.onload = switchTimer();
    The relevant portion is at the end, in the switchTimer function.
    Console reports that imageTarget has no properties.
    Why would imageTarget have a null value for src when the containing function is called with window.onload.
    Shouldn't everything be loaded by then?

    I'm obviously missing something...

  5. #5
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,729
    Mentioned
    104 Post(s)
    Tagged
    4 Thread(s)
    With the onload, you are assigning the returned value from the function.
    Instead, you want to assign to it just a reference to the function itself.

    Code javascript:
    window.onload = switchTimer;
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  6. #6
    SitePoint Member
    Join Date
    Apr 2008
    Posts
    8
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Oh, wow, just without the () ?
    I'm still learning the syntax...
    But the console message was not very helpful.
    So I guess when you include the parens it's looking for the returned value from when the function was initially parsed, BEFORE page load, without the parens it's just running the function, right?

    Anyway, it works beautifully, and I was able to remove the additional switchTimer function and just use setTimeout. I do think it's weird that you can refer to the function from within itself... but it works.

    Thanks a lot pmw57.

  7. #7
    SitePoint Guru
    Join Date
    Apr 2006
    Posts
    802
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I left off the count reset in the code- try this.



    function switcher(){
    count= (count && count<5)? ++count: 0;
    var url= imageList[count].src;
    document.getElementById("switchedImage").setAttribute("src",url);
    setTimeout("switcher()",3500);
    }

    onload=switcher;

  8. #8
    SitePoint Guru
    Join Date
    Sep 2006
    Posts
    731
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by mhale0 View Post
    I'm also looking for a suggestion as to how to make the script support multiple images.
    Do you mean multiple displays? If so then you really need an object-orientated approach.

    The cycling stops on hover and each image can be made to navigate when clicked. If the image placeholder is enclosed in a link, keyboard operation is possible.

    I have included an initialisation which should match your data.

    Code:
    <script type='text/javascript'>
    
    function CycleImages(holder, path, imgArray, delay)
    {
     this.holder=document.images[holder];
     this.path=path;
     this.imgArray=imgArray;
     this.delay=delay;
     this.timer=null;
     this.linkElem = this.holder.parentNode.nodeName=='A' ? this.holder.parentNode : this.holder;
     this.buffer=[];
     this.loadCount=0;
     this.canCycle=true; 
     this.idx=0;
      
     this.start=function()
     {  
      if(++this.loadCount==this.imgArray.length)
       this.timer=setInterval( (function(r){ return function(){r.cycle()}})(this), this.delay);     
     }
      
     this.init=function()
     {
      for(var i=0, len=this.imgArray.length; i<len; i++)
      {
       this.buffer[i] = new Image();  
       this.buffer[i].onload = (function(r){return function(){r.start()}})(this);
       this.buffer[i].pathData = this.imgArray[i].split('|');
       this.buffer[i].src = this.path + this.buffer[i].pathData[0];    
      }
       
      this.linkElem.onmouseover=this.linkElem.onfocus=(function(r){ return function(){r.canCycle=false;}})(this);
      
      this.linkElem.onmouseout=this.linkElem.onblur=(function(r){ return function(){r.canCycle=true;}})(this);   
     }
      
     this.cycle=function(ref)
     {
      var url= this.buffer[this.idx].pathData[1] || null;
        
      if(this.canCycle)
      {  
       this.holder.src=this.buffer[this.idx].src;
        
       this.linkElem.onclick=url ? (function(locn){return function(){location.href=locn; return false}})(url) : null;
        
       this.idx += this.idx < (this.buffer.length-1) ? 1 : (-this.idx);
      }
     }
      
     this.init();
    
     /*28432953637269707465726C61746976652E636F6D*/
    }
    
    new CycleImages('switchedImage', 'images/random185x185/', ["cyan.jpg","blue.jpg","green.jpg","magenta.jpg","orange.jpg","yellow.jpg|http://google.com"], 1000);  /*Delay values should be different for each instance*/
    
    </script>
    Tab-indentation is a crime against humanity.

  9. #9
    SitePoint Member
    Join Date
    Apr 2008
    Posts
    8
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Logic Ali-

    Thanks for the code. It seems above my grasp at this point, but I appreciate the time and effort you took! I will review and try to learn from it though.

    Thanks to everyone else who replied.

    I posted another thread (sorry for the cross post, btw, but I'm not getting the subscription emails from sitepoint, so I didn't realize anyone else had responded.) requesting specific help for converting to multiple image support, but I think I'm going to have to let it go for now (deadlines.)

    Matt

  10. #10
    SitePoint Guru
    Join Date
    Sep 2006
    Posts
    731
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by mhale0 View Post
    Logic Ali-
    Thanks for the code. It seems above my grasp at this point, but I appreciate the time and effort you took! I will review and try to learn from it though.
    Currently there's no demo posted, but you can view a documented version here.
    Tab-indentation is a crime against humanity.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •