Wanted to write my own form slider, Jquery/JS

Question

I would like a fair minded critique of this code to get some feedback. I am not trying to re-invent the mold here, and I know there are plugins galore I could have used to do what I am doing here, but the point was to do it myself so that I can learn more and become better.

I have included comments to try and best explain my train of thought through the process, please don't implicitly critique the comments, they are just to explain my thinking as I made this code.

code below does exactly what I want..functions great in all modern browsers, rip me apart and make it better so I can get better:

$(document).ready(function(){

var sliding = false

    $(function () {
        var pos = new Array(); //certainly a way to automate this more but I didn't mind putting exact numbers in for interval postions...please critique.
         pos[0] = 0;    
         pos[1] = 67;
         pos[2] = 133;
         pos[3] = 199;
         pos[4] = 267;
         pos[5] = 333;

        var target = $('span.slider')
        var targetToMove = null;
        var sliderCoordStart = null;
        var mouseMovement = null;       


        target.mousedown(function(c) { 
              c.preventDefault();//prevent chrome from turning cursor to text when dragging
              sliding = true
              targetToMove = $(this); 
              targetToMove.addClass('noselect');//make sure we cant drag  untargeted things all over the screen
              mouseCoordStart =  c.clientX;
              sliderCoordStart=  parseInt(targetToMove.css('marginLeft'));
              oncontextmenu = function() { 
              return false 
              }

        });
        $(document).mouseup(function(e) {
             oncontextmenu = function() { 
             return true 
             }
             sliding = false 
             document.onselectstart = function(){ 
             return true; 
             }
             if(targetToMove !=null){
             targetToMove.removeClass('noselect');//no need to have the non select class anymore
             }
             var compare = new Array();
             if(mouseMovement !=null ){          
             for(c=0;c<=5;c++){//do some math to make sure the slider ends up going to the closest interval postion
                 compare[c] = Math.abs(pos[c] - mouseMovement);
                }
             var goTo = Math.min.apply(Math, compare)
             var finalPos = compare.indexOf(goTo); 
             targetToMove.css('marginLeft', pos[finalPos])       

             }       
        })
        $(document).mousemove(function(e) {
            if(sliding){
            document.onselectstart = function(){ //probably a better/smoother way to chain that right?
            return false; 
            }
            sliderPos = targetToMove.css('marginLeft');
            mouseMovement = (e.pageX - mouseCoordStart)+sliderCoordStart;
                if(mouseMovement <= 0){//make sure we never exceed the left boundary
                    mouseMovement = 0;
                }
                if(mouseMovement >= 333){//make sure we never exceed the right boundary
                    mouseMovement = 333;
                }
              targetToMove.css('marginLeft', mouseMovement) 
            }
        })  
    })



});

Answer 1

Here are some suggestions to simplify the syntax:

• For pos, use an array literal: var pos = [0, 67, 133, 199, 267, 333]
  • Similar thing for compare: var compare = [];
• You're missing semi-colons after some statements

• You can make initializing compare simpler using Array.prototype.map (doesn't work on IE < 9):

  var compare = pos.map(function (e) {
      return e - mouseMovement;
  });
  

  • If you need to support IE < 9, I'd either implement map, or do basically what you're doing, but use Array.prototype.push instead:

    for(var c=0;c<=5;c++){//do some math to make sure the slider ends up going to the closest interval postion
        compare.push(Math.abs(pos[c] - mouseMovement));
    }
    

• You've got a lot of undefined variables that will default to the global scope:

  • sliderPos- not used
  • mouseCoordStart- dangerous because it might not be defined when it's used
  • oncontextmenu- not used anywhere
• 333 in your mousemove handler is magic, perhaps it should be pos[pos.length-1]?

I'm not going to comment on jQuery plug-in style (I don't use jQuery) or your implementation of a "form slider" (which I assume is an implementation of an HTML5 input range).

Hopefully this was useful!