Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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



});
share|improve this question

1 Answer 1

up vote 3 down vote accepted

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!

share|improve this answer
    
Thanks much for the feedback. So what should I do with mouseCoordStart? The other variables are indeed not used and missed removing them before posting. Actually was not going to use input range...just was going to js in the final value on some hidden form fields? –  absentx Nov 26 '12 at 22:01
    
First of all, declare it somewhere, there's no var mouseCoordStart, so this is global. Maybe stick it next to sliderCoordStart. I missed the if (sliding) conditional, so you should be fine. The formatting was funky, so it looked like it returned after creating the callback. Just declare it and you should be fine. –  tjameson Nov 27 '12 at 5:51

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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