1
\$\begingroup\$

I've been creating javascript things for a good few months now, as practise but whenever I use a functions I just create it and plug in the arguments. I'm never touching modules or classes or anything, and I just want to know how this code could be better, using modules and classes. Various function are sort of part of the same "family", such as launchTypeA, launchTypeB and launchSlider. launchTypeA, launchTypeB are similar, and both calllaunchSlider. I feel like this should be in some sort of module.

So what does this script do? It basically destroys and reappends a flexslider element on initial load and on subsequent breakpoint changes. This is so I can update the flexslider with different attributes for responsive design. (on the mobile breakpoint it maybe only displays one li, on larger breakpoints, maybe 5).

To find these slider elements, it uses a loop to automatically loop through all elements with a sliderElm class on the page and then iterate through the object later on. This is so the same script will work just as well with one slider as it will for hundreds on the same page. It also works with types of slider. Look at the example for a better idea:

http://jsfiddle.net/sWBcw/4/

The first three sliders are of TypeA. All type a sliders automatically slide at the standard speed. The last slider is of TypeB. All that's different about TypeB is its speed, but I could use any of any of these attributes to make TypeB vastly different.

Anyway, I just want to know what you think of this code (my first serious project) and how I can improve it. I mean I've kept it dry as possible, but it still feels different to code I read in github repositories. How should package this script? Make it into an object?

I would seriously appreciate some guidance.

$(document).ready(function() {

  // a bit of workaround but I quite like it. The css breakpoints apply a 
  // font-size of either 1px, 2px 3px or 4px to the empty div `#viewport`
  // from there we can work out the current breakpoint (1 for xs, 2 for sm, 3 for md and 4 for lg )

  function getBreakpoint(){
    // also cleans the breakpoint (some browsers add inverted commas)
    return $('#viewport').css('font-size').replace(/.*(1|2|3|4).*/g, '$1');
  }

  // stores the html of the slider in the variable, destroys the html and recreates it. Is there a better way to do this?
  function reappendSlider(sliderId){
    slider =  $(document).find( '#' + sliderId )
    sliderParent = $(document).find( '#' + slider.parent().attr('id') );
    sliderHtml = $(slider).html();     

    slider.remove();   

    sliderParent.append('<div id = "' +  sliderId + '"></div>');
    $(document).find( '#' + sliderId ).append( sliderHtml );

  }

  function launchSlider(sliderId, a){

    var o = {
      'namespace' : 'flex-',
      'selector' : '.slides > li',
      'animation' : 'slide',
      'easing' : 'swing',
      'direction' : 'horizontal', 
      'reverse' : false,
      'animationLoop' : true, 
      'smoothHeight' : false,
      'startAt' : 0,
      'slideshowSpeed' : 7000,
      'animationSpeed' : 600,
      'initDelay' : 0,
      'randomize' : false,
      'pauseOnAction' : true,
      'pauseOnHover' : false,
      'useCSS' : true,
      'touch' : false,
      'video' : false,
      'controlNav' : false,
      'directionNav' : false,
      'prevText' : 'Previous',
      'nextText' : 'Next',
      'keyboard' : false,
      'multipleKeyboard' : false,
      'mousewheel' : false,
      'pausePlay' : false,
      'pauseText' : 'Pause',
      'playText' : 'Play',
      'controlsContainer' : '',
      'manualControls' : '',
      'sync' : '',
      'asNavFor' : '',
      'itemWidth' : '',
      'itemMargin' : '',
      'minItems' : 0,
      'maxItems' : 0,
      'move' : 1,
      'start' : function(){},
      'before' : function(){},
      'after' : function(){},
      'end' : function(){},
      'added' : function(){},
      'removed' : function(){}
    }

    $.extend(o, o, a);

    o['minItems'] = o['maxItems']
    o['itemWidth'] = $(document).find( '#' + sliderId ).width() / o['maxItems']

    console.log( o['move'] );

    $(document).find( '#' + sliderId ).flexslider({
      namespace : o['namespace'],
      selector : o['selector'],
      animation : o['animation'],
      easing : o['easing'],
      direction : o['direction'], 
      reverse : o['reverse'],
      animationLoop : o['animationLoop'], 
      smoothHeight : o['smoothHeight'],
      startAt : o['startAt'],
      slideshowSpeed : o['slideshowSpeed'],
      animationSpeed : o['animationSpeed'],
      initDelay : o['initDelay'],
      randomize : o['randomize'],
      pauseOnAction : o['pauseOnAction'],
      pauseOnHover : o['pauseOnHover'],
      useCSS : o['useCSS'],
      touch : o['touch'],
      video : o['video'],
      controlNav : o['controlNav'],
      directionNav : o['directionNav'],
      prevText : o['prevText'],
      nextText : o['nextText'],
      keyboard : o['keyboard'],
      multipleKeyboard : o['multipleKeyboard'],
      mousewheel : o['mousewheel'],
      pausePlay : o['pausePlay'],
      pauseText : o['pauseText'],
      playText : o['playText'],
      controlsContainer : o['controlsContainer'],
      manualControls : o['manualControls'],
      sync : o['sync'],
      asNavFor : o['asNavFor'],
      itemWidth : o['itemWidth'],
      itemMargin : o['itemMargin'],
      minItems : o['minItems'],
      maxItems : o['maxItems'],
      move : 1,
      start : o['start'],
      before : o['before'],
      after : o['after'],
      end : o['end'],
      added : o['added'],
      removed : o['removed']
    });    

  }

  function launchTypeB(id, maxItems, itemMargin){
    slider = findSlider(id)
    sliderParent = slider.parent().parent()
    sliderLeftNav = sliderParent.find('.left-nav')
    sliderRightNav = sliderParent.find('.right-nav')

    if(initial == false){
      reappendSlider(id)
    }

    sliderLeftNav.click(function(){
      findSlider( id ).flexslider('prev');
    });

    sliderRightNav.click(function(){
      findSlider( id ).flexslider('next');
    }); 

      launchSlider(id, {'maxItems' : maxItems, 'itemMargin' : itemMargin, 'slideShow' : true, 'slideshowSpeed': 10 });
  }   

  function launchTypeA(id, maxItems, itemMargin){

    if(initial == false){
      reappendSlider(id)
    }

    sliderLeftNav.click(function(){
      findSlider( id ).flexslider('prev');
    });

    sliderRightNav.click(function(){
      findSlider( id ).flexslider('next');
    }); 

    launchSlider(id, {'maxItems' : maxItems, 'itemMargin' : itemMargin });
  }


  function findSlider(id){
    return $(document).find( '#' + id )
  }

  function loopSliders(){


    $.map( typeASlidersIds, function( value, type ){    


      $.map( value, function( value, key){


          switch(type){
            case 'sliderTypeA':

                  console.log(breakpoint);

              switch(breakpoint){
                case 'xs':

                  console.log( 'xs launched' );
                  launchTypeA(value, 1,0);

                break;

                case 'sm':

                  launchTypeA(value, 2,0);

                break;

                case 'md':

                  launchTypeA(value, 2,0);

                break;

                case 'lg':

                  launchTypeA(value, 2,0);

                break;
              }



            break; 

            case 'sliderTypeB':
                switch(breakpoint){
                    case 'xs':

                      launchTypeB(value, 1,0);

                    break;

                    case 'sm':

                      launchTypeB(value, 2,0);

                    break;

                    case 'md':

                      launchTypeB(value, 2,0);

                    break;

                    case 'lg':

                      launchTypeA(value, 2,0);

                    break;
              }

            break;
          }


      });
    }); 
  }

  function xsBreakpoint(){
    breakpoint = 'xs'
    loopSliders();
  }

  function smBreakpoint(){
    breakpoint = 'sm'
    loopSliders();
  }

  function mdBreakpoint(){
    breakpoint = 'md'
    loopSliders();
  }

  function lgBreakpoint(){
    breakpoint = 'lg'
    loopSliders();
  }

  function launchBreakpoint(breakpoint){


    switch( breakpoint ){
      case '1':
        xsBreakpoint();
      break;
      case '2':
        smBreakpoint();
      break;
      case '3':
        mdBreakpoint();
      break;
      case '4':
        lgBreakpoint();
      break;
    }

    initial = false
  }

  /* begin */
  typeASlidersIds = {}
  currentBreakpoint = getBreakpoint()
  initial = true
  timer = 0


  $('.sliderElm').each(function( key, slider ){
    type_class = $(slider).attr('class').split(' ')[0]


    if( typeASlidersIds.hasOwnProperty(type_class) == false) {
      typeASlidersIds[type_class] = []
    }

//    typeASlidersIds[type_class].push(slider);
    typeASlidersIds[type_class].push($(slider).attr('id'));
  });



  /* start now */
  launchBreakpoint(currentBreakpoint);

  /* start after reszie */
  $(window).resize(function(){
      clearTimeout(timer);
      timer = setTimeout( function(){
        // below is run every 1/4 of a second!
          var newBreakpoint = getBreakpoint(); 

          $.map( typeASlidersIds, function( typaASliderId ){
            findSlider(typaASliderId).resize();
          });  

          if(newBreakpoint != currentBreakpoint){

            launchBreakpoint( newBreakpoint );

            currentBreakpoint = newBreakpoint
          }
        // above is run every 1/4 of a second! 
      }, 250)
    });

});
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

On the first pass the code was pretty easy to follow but are all those global variable intended?

Globals I noticed in my look through... May be more.

breakpoint
typeASlidersIds
currentBreakpoint
initial
timer
slider
sliderParent
sliderHtml

I would add a var breakpoint, typeASlidersIds, ... after your doc.ready()

Also the helpers you use in launchBreakpoint seem pretty redundant. I would personally drop the helpers and write that function:

function launchBreakpoint(brkpnt){
    switch( brkpnt ){
      case '1':
        breakpoint = 'xs'
      break;
      case '2':
        breakpoint = 'sm'
      break;
      case '3':
        breakpoint = 'md'
      break;
      case '4':
        breakpoint = 'lg'
    }

    loopSliders();
    initial = false
}

One last thing before I head out... These are terrible variable names and, moreover, extending an object with its self is pointless

$.extend(o, o, a);

To answer the actual question OP seems to be posing -- the pattern is fine I wouldn't change it but I would expose whatever you want to expose explicitly into a namespace using this format (window or whatever namespace):

window.myglobal = {
   prop1: ...
   prop2: ...
}

That pattern would closely match The javascript revealing pattern

\$\endgroup\$
7
  • \$\begingroup\$ Thanks, are the only bad variable names o and a? Is this because they don't describe what they contain? \$\endgroup\$
    – Starkers
    Commented Mar 21, 2014 at 19:57
  • \$\begingroup\$ Also, could you emphasize a lot more on your last point? An example using my code would be of great help :) And globals are just variables declared at the global scope? So what is the benefit of using var? Stop strange bugs by not letting them get inside functions? \$\endgroup\$
    – Starkers
    Commented Mar 21, 2014 at 20:13
  • \$\begingroup\$ Global variables are poor practice for various readability, maintainability, performance, etc. see this SO post. As far as the last point is concerned theres plenty of resources I can point you to if the linked one is insufficent (its my personal favourite) but I'm not going to write your code as I don't know whats supposed to be exposed \$\endgroup\$
    – megawac
    Commented Mar 21, 2014 at 20:53
  • \$\begingroup\$ Variable names should be descriptive.. I don't know what they mean but I'm assuming id probably call o config or something and a options \$\endgroup\$
    – megawac
    Commented Mar 21, 2014 at 20:54
  • \$\begingroup\$ Cool...I'll admit I don't really know what you mean by exposed? \$\endgroup\$
    – Starkers
    Commented Mar 21, 2014 at 20:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.