Question

I need suggestions about writing better code in Revealing Module Pattern way. I have followed tutorial http://weblogs.asp.net/dwahlin/archive/2011/09/05/creating-multiple-javascript-objects-when-using-the-revealing-module-pattern.aspx which help me a lot to understand basics of this pattern. I am trying to create basic image slider. Please check jsfiddle link,

http://jsfiddle.net/sWtDf/

var Slider = window.Slider = window.Slider || {};

    Slider = (function($){

        var $reelWrap = $('.fnSlider'),
            $reel = $('.fnSlider').children('ul'),
            $slide = $reel.children('li'),
            slideWidth = $slide.width(),
            numSlides = $slide.length,                
            reelWidth = numSlides*slideWidth,
            $prev = $('.prev'),
            $next = $('.next'),

            init = function(){   
                pageLoad();               
                nextSlide();
                prevSlide();
            },

            pageLoad = function(){
                var index = 2;                                  
                $reel.css('width', reelWidth);
                $slide.eq(index).addClass('fnActive');     
                $reel.css('left', -(index*slideWidth));
            }

            nextSlide = function(){ 

                $next.click(function(e){
                    e.preventDefault();

                    var index = $reel.children('.fnActive').index() + 1;
                    var scroll = index * slideWidth;

                    if(index < numSlides){
                        $reel.animate({left: -(scroll)}).children().removeClass('fnActive');
                        $slide.eq(index).addClass('fnActive');
                    }else{
                       $reel.animate({left: 0}).children().removeClass('fnActive'); 
                       $slide.eq(0).addClass('fnActive');
                    }

                });

            },

            prevSlide = function(){ 

                $prev.click(function(e){
                    e.preventDefault();

                    var index = $reel.children('.fnActive').index() - 1;
                    var scroll = index * slideWidth;
                    var lastScroll = (numSlides-1) * slideWidth;

                    if(index == "-1"){
                         $reel.animate({left: -(lastScroll)}).children().removeClass('fnActive');
                         $slide.eq(-1).addClass('fnActive');
                    }else{
                        $reel.animate({left: -(scroll)}).children().removeClass('fnActive');
                        $slide.eq(index).addClass('fnActive');  
                    }

                });

            }

            return {
                init: init
            }


    })(jQuery);

    $(function(){

        Slider.init();

    });

What I want to know is -

1) is there better way to write same carousel functionality after code review

2) how to create multiple objects (instances) I tried -

        var slider1, slider2;

        slider1 = Slider();
        slider2 = Slider();

        slider1.init();
        slider2.init();

which causes error TypeError: Slider is not a function

3) is it a correct way to keep global and private functions

4) suggestios if any

Thanks for your time going through this :)

Was it helpful?

Solution

If you want to create multiple instances, I suggest that you return a constructor function in your module:

var Slider = (function($){

    function slider(id){  // <- 'foo' is an example of a constructor argument 
                          // (1 and 12 below when the instances are created)

        this.id = id; // <- something specific for each instance
        // this.$reelWrap = ...
    }

    slider.prototype = {

        init: function(){
            this.pageLoad();
        },

        pageLoad: function(){
             console.log('pageLoad called from instance with id', this.id);
        },

        getId: function(){
            return this.id; // <- 'this' refers to the current instance
        }
    };

    return slider;

})(jQuery);

var slider1 = new Slider(1);
var slider2 = new Slider(12);

console.log(slider1.getId());

var Vehicle = function(engine, speed){
    this.engine = engine;
    this.speed = speed || "786km/Hr";    
} 

Vehicle.prototype.engineInfo = function(){
    console.log("this vehicle has engine " + this.engine);
}

var Porsche = function(engine, speed, carName){                                       

    Vehicle.apply(this, arguments);   

// I stucked here and got answer 
// http://stackoverflow.com/questions/8605788/
// javascript-call-and-apply-functions-only-called-on-first-argument
// it means Vehicle functions arguments got copied here am I right :)

// J: The point is to call the 'base class' contructor with the arguments
// that you provide when you create a new Porsche as well.
// In other words, engine and speed will be set correctly in the 
// Vehicle constructor when you create a new Porsche()

    this.carName = carName;
}

Porsche.prototype = Object.create(Vehicle.prototype);
// Porsche.prototype = new Vehicle();
// http://stackoverflow.com/questions/4166616/
// understanding-the-difference-between-object-
// create-and-new-somefunction-in-j
// hmm need    more time to go into this :) 

// J: The difference is that if you just do Porsche.prototype = new Vehicle();
// The Vehicle constructor will be called even if you don't create an instance
// of Porsche

Porsche.prototype.constructor = Porsche;
// I stucked here and got http://stackoverflow.com/questions/9343193/
// why-set-prototypes-constructor-to-its-constructor-function

// J: I would say that it's good practice to do it, but I've personally never
// found any uses of that property.

Porsche.prototype.speedInfo = function(){
    console.log("this vehicle has speed " + this.speed);
}

var cayMan1 = new Porsche("Engine UV", "500km/hr", "CayMan");
var cayMan2 = new Porsche("Engine Ultra", "100km/hr", "911"); 

cayMan2.engineInfo();        
cayMan2.speedInfo();

OTHER TIPS

2) You are not exposing Slider


var Slider = (function($){

  return function() {
    var $reelWrap = $('.fnSlider'),
    .........
  }
}(jQuery))

3) Generally, your scoping is good.. you are not exposing more than you have to

4) You are missing question 4!!

5) You should generally buy low and sell high given the opportunity

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top