2

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 :)

Pravin W
  • 2,451
  • 1
  • 20
  • 26

2 Answers2

5

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();
Johan
  • 35,120
  • 54
  • 178
  • 293
  • @ Johan Thanks for your reply and detailed code snippet :) going further I did this ex-1 http://jsfiddle.net/LXNgD/2/ - is it correct or anything better can be done with this? also I tried like ex-2 http://jsfiddle.net/nex2k/ which helps me to extend and overwrite methods. with ex-1 i couldn't able to overwrite slider1.nextSlide method any suggestions – Pravin W Feb 21 '14 at 13:49
  • @PravinVaichal Well, in brief, you seem to have got the hang of it. If you want to extend and overwrite methods I suggest that you create a "base" slider and let your other types inherit from it. Let me know if you want an example. – Johan Feb 21 '14 at 16:58
  • @ Johan first of all thanks for your valuable time you put into this. an example would be great :) to play with as per my coding knowledge this piece of functionality can be achieved via Revealing Module Pattern, Obeject oriented (using Prototype creating instances and its flexible), jQuery Plugin if you gotta time can you explain - for any js functionality how can you decide which of the method is beneficial any reasons if specific. :) – Pravin W Feb 21 '14 at 19:07
  • 1
    @PravinVaichal No problem. I just got home from the bar so bear with me here. Not sure if this is the best example, but hey... http://jsfiddle.net/ERW4v/ Let me know if you have any questions. I didn't quite get all the things you asked, but don't confuse this with a jQuery plugin. Plugins can be useful to an extent, but I prefer to do something like this instead. You can always pass the set of elements to the constructor, rather than doing `$('.whatever').plugin()`. In other words, I would stick with the pattern that you chose – Johan Feb 21 '14 at 23:26
  • @ Johan - Thanks for example very helpful and apologise for late reply. I gone through example and tried to implement as - http://jsfiddle.net/JGb8R/2/ also I came across - Revealing Prototype Pattern (your first answer ) I explored more into it - http://jsfiddle.net/BV954/ which I guess most help full in case of multiple instances, extend prototype chain, namespace etc. do you have twitter/email to follow? OOJS is now more clear to me – Pravin W Feb 26 '14 at 09:58
  • @PravinVaichal Np. I've tried to answer your questions in my update. Regarding your second example: I guess it could woruk, but keep in mind that you're overwriting the existing prototype of `Ford.MiniVan`. It doesn't matter in this case, but it could have inherited methods from other prototypes for example. And I don't use twitter :] – Johan Feb 26 '14 at 10:13
  • "overwriting the existing prototype of Ford.MiniVan" trying to figure it out - any code example of it that shows overwrite if you gotta time. I thought I could use it like Yahoo in real project to keep it modular and reusable. like Yahoo.slider, Yahoo.accordion etc – Pravin W Feb 26 '14 at 10:28
  • @PravinVaichal Here are 2 examples where inheritance wouldn't work: http://jsfiddle.net/BV954/1/, http://jsfiddle.net/BV954/2/. To recreate the YAHOO functionality I would create a base object called YAHOO, which both the `Slider` and `accordion` inherits from. – Johan Feb 26 '14 at 11:52
  • However, if the module only will be used once per page, I would return and object from the module, rather than a function. – Johan Feb 26 '14 at 11:58
1

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

ilan berci
  • 3,883
  • 1
  • 16
  • 21
  • @ ilan berci Thanks for your time and comments question numbering is corrected will you please elaborate 5) I didn't understood completely – Pravin W Feb 21 '14 at 13:50