Anyone care to review this JS code for a custom scroller?

Question

Hi Guys

I'm by no means a programmer... I've never really taken a comp sci class, don't know much theory, but I still hack together code that usually works. I just don't know how ugly it actually is. I recently wrote this (simple) bit of JS and was wondering I could get some feedback on it...

If this isn't an appropriate place for it, let me know. Thanks.

--Will

    /*
    @author: Will Cavanagh
    @date: 2009-09-14
    * Custom scroll box
    */
    var customScroller = {
 intRegex : /^\d+$/,
 maxScroll : 0,
 inited : false,
 upColor : "#FFF",
 downColor : "#FFF", 
 defSpeed : "#FFF",

 //init function -- sets config values and initallizes jQuery slider.
 //@param options : object containing set up parameters
 //@return null
 init : function(options) {
  //if there are no options/colors specified, make empty object
  if(!options) { options = new Object(); };
  if(!options.scrollerColor) { options.scrollerColor = new Object(); };

  //assign variables, use defaults if not defined.
  var width = this.intRegex.test(options.width) ? options.width : 500
  var height = this.intRegex.test(options.height) ? options.height : 300;
  var vertical = options.vertical == null ? true : options.vertical;
  upColor = options.scrollerColor.upColor == null ? '#4a4a4a' : options.scrollerColor.upColor;
  downColor = options.scrollerColor.downColor == null ? '#333' : options.scrollerColor.downColor;
  var bkgdColor = options.scrollerColor.bkgdColor == null ? '#848484' : options.scrollerColor.bkgdColor;
  defSpeed = options.defaultSpeed == null ? '1000' : options.defaultSpeed;

  //set content width before measuring
  jQuery("#content-scroll").css({width: width});

  //get height of content, subtract height of pane to be shown in
  maxScroll = jQuery("#content-scroll").height() - height;
   //set the height of pane to hold content.  This is done after measuring content height for browser compatability reasons
  jQuery("#content-scroll").css({width: width, height: height});
  if(this.vertical) {
   var orientation = 'vertical';
  } else {
   var orientation = 'horizontal';
  }

  //create the JQuery.UI slider
  jQuery("#content-slider").slider({
   value: 100,
   orientation: 'vertical',
      animate: false,
     change: customScroller.handleSliderChange,
      slide: customScroller.handleSliderSlide,
     start: customScroller.handleSliderStart,
      stop: customScroller.handleSliderStop
     });

     jQuery(".ui-slider-handle").css({background:upColor});
     jQuery("#slider-bkg").css({background:bkgdColor});

     $("#content-scroll").mousewheel(function(objEvent, intDelta){
   scroll(intDelta * -30, 0);
  });

  inited = true;
 },

 //animates a scroll to the beginning of the content
 //@return null
 goTop : function() {
  if(!inited) { return };
  var scrollto = 0;
  jQuery("#content-scroll").animate({scrollTop: scrollto}, {queue:false, duration:defSpeed});
  jQuery("#content-slider").slider('option', 'value', 100*(1-(scrollto/maxScroll)));
 },


 //handler function bound to a slider change event.
 //@param e : event
 //@param ui : slider ui object
 //@return null
 handleSliderChange : function(e, ui) {
  if(!inited) { return };
  jQuery("#content-scroll").animate({scrollTop: ((100-ui.value) / 100) * (maxScroll) }, {queue:false, duration:defSpeed});
 },

 //handler function bound to a slider slide event.
 //@param e : event
 //@param ui : slider ui object
 //@return null
 handleSliderSlide : function(e, ui) {
  if(!inited) { return };
  jQuery("#content-scroll").attr({scrollTop: ((100-ui.value) / 100) * (maxScroll) });
 },

 //handler function bound to a slider start of slide event.
 //@return null
 handleSliderStart : function() {
  if(!inited) { return };
  jQuery(".ui-slider-handle").css({background:downColor});
 },

 //handler function bound to a slider end of slide event.
 //@return null
 handleSliderStop : function() {
  if(!inited) { return };
  jQuery(".ui-slider-handle").css({background:upColor});
 },


 //scroll by a given amount.
 //@param amt : amount to scroll by
 //@param speed : sroll animation speed, defaults to default speed defined in init params
 //@return null
 scroll : function(amt, speed) {
  if(!inited) { return };
  if(!speed) { speed = defSpeed; }
  var scrollto = jQuery("#content-scroll").scrollTop() + amt;
  if(scrollto > (maxScroll - 20)) scrollto = maxScroll; //near or past end of content, scroll to end
  if(scrollto < 20) scrollto = 0; //near or past beginning of content, scroll to beginning

  jQuery("#content-scroll").animate({scrollTop: scrollto}, {queue:false, duration:speed}); //animate content scroll
  jQuery("#content-slider").slider('option', 'value', 100*(1-(scrollto/maxScroll))); //update slider position
 }
    }

Answer 1

Few tips:

• new Object(); can be safely replaced with object literal - { }.
• You're missing some quotes after statements (perhaps run it through JSLint).
• You have undeclared variables, e.g. upColor and downColor. Declare them with var.
• You have only some of the values in "config". Why are the rest inline?
• It's better to avoid "magic numbers" such as 20 in your example (scrollto < 20). Define them separately under descriptive names.
• Some of the selector strings - e.g. "#content-scroll" - are repeated throughtout entire script; it's probably best to take them out into config (making things more extensible and DRY at the same time).
• Comparison with null seems unnecessary in this case (options.scrollerColor.upColor == null). I would drop null and take advantage of implicit type conversion instead (which would catch empty strings too!)

Answer 2

In addition to what kangax sayd, this is a bit clumsy way to define defaults:

var width = this.intRegex.test(options.width) ? options.width : 500;
var height = this.intRegex.test(options.height) ? options.height : 300;
var vertical = options.vertical == null ? true : options.vertical;

A better approach is to use the || operator:

var width = options.width || 500;
var height = options.height || 300;
var vertical = options.vertical || true;

An even better way would be a function that takes an object with default options and combines these with actual supplied options:

var defaultOptions = {width: 500, height: 300, vertical: true};
var options = applyDefaults(defaultOptions, options);

Also...

You might want to consider dropping those if(!inited) { return } lines completely, or replacing them with if(!inited) { alert("not inited"); } because otherwise you are masking errors. When somebody tries to use your customScroller but forgets to run init(), then currently the thing just doesn't work, doesn't even give an error, and it's going to be pretty hard to find out the reason why everything just silently fails. It's better to fail with huge noise.

And more...

It seems that you really need to consider adopting a more object-oriented approach. Currently you have all these global variables inited, upColor, downColor, etc, that really-really want to be instance variables. Say, something like this:

var CustomScroller = function(options) {
  this.options = options;
};
CustomScroller.prototype = {
  init: function() {
    ...
    this.inited = true;
  },
  goTop: function() {
    if (!this.inited) { alert("Not inited!"); }
    ...
  }
};

var myScroller = new CustomScroller({width: 100, height: 300, ...});
myScroller.init();
myScroller.goTop();

Answer 3

Mainly, don't be afraid of whitespace. Putting the statements of a block on a different line than the expression makes it easier to see where the expression ends and the statements begin.

example:

if(expression) {
    statement;
}

vs

if(expression){ statement; }

Also it's a good practice to always use parentheses in your block statements, this prevents a lot of bugs.

example

if(expression) {
  statement;
  statement;
}

vs

if(expression) 
statement;
statement; //woops

You have a lot of if- not -return constructions in there, where if-do would be less lines and clearer.

example

if(!expression) {
  return;
}
statement;

vs

if(expression) {
  statement;
}