Problem with my variable scope in JavaScript I think

Question

I have quickly coded up a sort of product display thing that gets half of it's input from the page, and the other half from an AJAX query.

Here is the code

function productDisplay() {


    products = [];

    this.index = 0;

    setupProductDisplay();

    processListItems();

    showProduct();

    function setupProductDisplay() {

     var productInfoBoxHtml = '<div id="product-info"><h3 class="hide-me"></h3><span id="dimensions" class="hide-me"></span><div id="product-gallery"><img alt="" src="" /></div><ul id="product-options" class="hide-me"><li id="spex-sheet"><a href="" rel="external">Download full spex sheet</a></li><li id="enlarge-image"><a href="" rel="lightbox-gallery">Enlarge image</a></li></ul><div id="product-description" class="hide-me"></div><span id="top"></span><span id="bottom"></span><span id="side"></span><span class="loading"></span></div>';
     $('#products').after(productInfoBoxHtml);
    }

    function processListItems() {

     $('#products > li')
      .append('<span class="product-view">View</span>')
      .filter(':even')
      .addClass('even')
     .end()
      .each(function() {

       products.push({
        id: $(this).find('h3').html(),  
        title: $(this).find('h3').html(),
        dimensions: $(this).find('.dimensions').html(),
        description: $(this).find('.product-description').html()
       });

     })
     .find('.product-view')
      .click(function() {

       var $thisListItem = $(this).parents('ul li');

       var index = $('#products > li').index($thisListItem);

       this.index = index;

       showProduct();


      });

    };


    function showProduct() {

       var index = this.index;

       console.log('INDEX = ' + index);

     // hide current data
      $('#product-info')
      .show()
      .find('.hide-me, #product-gallery')
       .hide()
      .parent()
       .find('.loading')
       .show();



      // get data contained in the page

      $('#product-info')
      .find('h3')
       .html(products[index].title)
      .parent()
      .find('#dimensions')
       .html(products[index].dimensions)
      .parent()
      .find('#product-description')
       .html(products[index].description)


      // get id & then product extra info

      var id = $('#products > li').eq(index).attr('id').replace(/id-/, '');




      var downloadPath = PATH_BASE + 'downloads/';

      var imagePath = PATH_BASE + 'images/products/'

      $.getJSON(PATH_BASE + 'products/get/' + id + '/',
             function(data){           
         var file = ''; 
         var images = [];

         file = data.file;

         images = data.images;

         // show file list item if there is a file
         if (file) {
        $('#spex-sheet').show().find('a').attr( { href: downloadPath + file  } ); 
         } else {       
        $('#spex-sheet').hide();
         }

         // image gallery



         if (images.length != 0) {
          $('#product-gallery').show();
          // preload image thumbnails
        $.each(images, function(i, image){
         var img = new Image();
         img.src = imagePath + 'thumb-' + image;
         img = null;
        });

        // set first image thumbail and enlarge link
        if (images[0]) {
         $('#enlarge-image').show().find('a').attr({ href: imagePath + images[0] });
         $('#product-gallery img').attr ( { src: imagePath + 'thumb-' + images[0]} )

        }

        console.log(images);

        // setup gallery

        var currentImage = 0;

        clearInterval(cycle);

        console.log(cycle);



        var cycle = setInterval(function() {
         console.log(currentImage + ' = ' + index);
         if (currentImage == images.length - 1) {   
          currentImage = 0;    
         } else {    
          currentImage ++;    
         };

         var obj = $('#product-gallery');

         var imageSource = imagePath + 'thumb-' + images[currentImage];   
         obj.css('backgroundImage','url(' + imageSource  +')');  
         obj.find('img').show().fadeOut(500, function() { $(this).attr({src: imageSource}) });
         $('#enlarge-image a').attr({ href: imagePath + images[currentImage] });   
        }, 5000);


        // setup lightbox
        $("#enlarge-image a").slimbox({/* Put custom options here */}, null, function(el) {
         return (this == el) || ((this.rel.length > 8) && (this.rel == el.rel));
        });



         } else {
          // no images

        $('#enlarge-image').hide();
        $('#product-gallery').hide();

         };


         // show the product info
         $('#product-info')
          .find('.hide-me')
         .remove('#product-gallery, #spex-sheet')
          .show()
        .parent()
        .find('.loading')
         .hide();

      });


    };




};

The important function is showProduct(). Now generally I don't write JS like this, but I decided to give it a go. My problem is, that when a user clicks a 'more' button, and it displays the prouduct, it doesn't reset the simple slideshow (the images var is reset, I think it has to do with the setInterval() maybe, or it seems it's making a new instance of showProduct() everytime).

Does anyone know what I'm doing wrong?

Thanks

Answer 1

If you don't define your variables with var (e.g. var images = ...;) then they will be considered global variables (members of the window object).

If you define them with var then they are visible to the whole function (even before the variable is declared) they are declared in.

I can't immediately see what the problem is, but I would recommend minimizing the scope of your variables - if they don't need to be global then make sure they aren't global.

Answer 2

I had to reformat your code to really understand what was going on. Anyway, I found the problem with the code.

As you guessed correctly, problem is with the scope but not with the variable 'images' but with variable 'cycle'. Why?

This line

var cycle = setInterval(function() {

Always creates a new local cycle variable (notice the 'var') which is not accessible when showProduct gets called the second time. This means that this line

clearInterval(cycle);

is essentially useless as it always passes null to the clearInterval function and doesn't clear anything. This means that as you keep clicking on 'more', you are creating more and more setInterval function calls, never clearing the old ones.

Anyway, I have refactored your code a little bit, I think this should work as expected. The changes I did are:

1. Removed this.index variable. It's better to pass 'index' to showProduct instead of setting this.index before showProduct method call and making showProduct use that variable. Also, why did you prefix the variable with 'this'?

2. Declared cycler variable outside the scope of showProduct, local to the productDisplay method. This insures that you can access cycler during different showProduct calls.

3. Created smaller functions named showFile, showGallery, showProductInfo to make it easier to understand/maintain code.

Let me know if you have any questions OR if the code still doesn't work.

function productDisplay() {

    //Instead of keeping this.index variable, it's better to make showProduct function
    //take index variable. 

    products = [];
    setupProductDisplay();
    processListItems();

    //We have to define cycler outside the showProduct function so that it's maintained
    //in between showProduct calls. 
    var cycler = null;

    showProduct(0);

    function setupProductDisplay() 
    {
        var productInfoBoxHtml = '<div id="product-info"><h3 class="hide-me"></h3><span id="dimensions" class="hide-me"></span><div id="product-gallery"><img alt="" src="" /></div><ul id="product-options" class="hide-me"><li id="spex-sheet"><a href="" rel="external">Download full spex sheet</a></li><li id="enlarge-image"><a href="" rel="lightbox-gallery">Enlarge image</a></li></ul><div id="product-description" class="hide-me"></div><span id="top"></span><span id="bottom"></span><span id="side"></span><span class="loading"></span></div>';
        $('#products').after(productInfoBoxHtml);
    }

    function processListItems() 
    {
        $('#products > li')
      .append('<span class="product-view">View</span>')
            .filter(':even')
            .addClass('even')
      .end()
            .each(
       function() 
       {
        products.push({
            id: $(this).find('h3').html(),          
            title: $(this).find('h3').html(),
            dimensions: $(this).find('.dimensions').html(),
            description: $(this).find('.product-description').html()
          });

             })
      .find('.product-view')
            .click( function()
        {
                        var $thisListItem = $(this).parents('ul li');
                        showProduct($('#products > li').index($thisListItem));

        }
       );

    };

    function showFile(file)
    {
     if (file)
     {
      $('#spex-sheet').show().find('a').attr( { href: downloadPath + file  } );       
        } 
     else 
     {                                      
      $('#spex-sheet').hide();
        }
    }

    function showGallery(images)
    {
     if(! images || !images.length || images.length == 0)
     {
      $('#enlarge-image').hide();
            $('#product-gallery').hide();
      return;
     }

     $('#product-gallery').show();

     $.each(images, 
       function(i, image)
       {
        var img = new Image();
                    img.src = imagePath + 'thumb-' + image;
                    img = null;
                });

     // set first image thumbail and enlarge link
        if (images[0])
     {
      $('#enlarge-image').show().find('a').attr({ href: imagePath + images[0] });
            $('#product-gallery img').attr ( { src: imagePath + 'thumb-' + images[0]} )
     }

     var currentImage = 0;
     clearInterval(cycler);

     cycler = setInterval(
       function() 
       {
        currentImage = currentImage == images.length - 1 ? 0 : currentImage++;
        var obj = $('#product-gallery');

                    var imageSource = imagePath + 'thumb-' + images[currentImage];                  
                    obj.css('backgroundImage','url(' + imageSource  +')');          
                    obj.find('img').show().fadeOut(500, function() { $(this).attr({src: imageSource}) });
                    $('#enlarge-image a').attr({ href: imagePath + images[currentImage] });                 
                }, 5000);



        $("#enlarge-image a").slimbox({/* Put custom options here */}, null, function(el) {
                                        return (this == el) || ((this.rel.length > 8) && (this.rel == el.rel));
                                });

    };

    function showProductInfo()
    {
     $('#product-info')
      .find('.hide-me')
       .remove('#product-gallery, #spex-sheet')
                .show()
      .parent()
       .find('.loading')
                .hide();
    }

    function showProduct(index) 
    {
     $('#product-info')
      .show()
            .find('.hide-me, #product-gallery')
       .hide()
            .parent()
       .find('.loading')
                .show();

     // get data contained in the page
     $('#product-info')
      .find('h3')
       .html(products[index].title)
      .parent()
       .find('#dimensions')
        .html(products[index].dimensions)
                .parent()
                .find('#product-description')
        .html(products[index].description)

     // get id & then product extra info
        var id = $('#products > li').eq(index).attr('id').replace(/id-/, '');

     var downloadPath = PATH_BASE + 'downloads/';
        var imagePath = PATH_BASE + 'images/products/'

        $.getJSON(PATH_BASE + 'products/get/' + id + '/',
      function(data)
      {           
       showFile(data.file);
       showGallery(data.image);
       showProductInfo();

      });

    };




};