Window-scrolling controller

Question

I generally feel that the pattern I use for development is kind of wrong. I start with simple functionality, then when I realize that some pieces can be reused I usually start to write "scope root level" functions in order to be more DRY. I usually place these functions at the bottom of my "scope". Would these functions (changeColumn, in this case) better be methods of main object? Would this script better be organized in a more object-oriented way? Are there major problems with my coding style?

Unfortunately, I work directly with back-end programmers who don't give a damn about JS and so I am left with myself at this. I am trying to look at "rock stars", but they mainly write tools, not simple interaction functionality, so for this kind of scripts I rarely get to see examples of really good code to learn from.

/** Depends on: 
 * - jQuery (tested with 1.5.1) 
 * http://jquery.com/
 * - jQuery scrolTo (tested with 1.4.2)
 * http://flesler.blogspot.com/2009/05/jqueryscrollto-142-released.html
 **/
// Main obj
omami = {
 init: function() {
 // scrollable area
 var content = $('#content').css('overflow-x', 'scroll'),
 // flag identifying scroll event
 didScroll,
 // flag identifying if scroll event 
 // is caused by mouse / trackpad etc.
 // i.e. user action
 userScroll,
 // flag identifying if scroll event 
 // is caused by script action
 scriptScroll,
 // content sections
 cols = content.find('.column'),
 // hash table to store initial columns x-axis positions
 positionsMap = {},
 currentColHash,
 // Checks if first argument is bigger than second 
 // and smaller than third
 isInRange = function(num, a, b) {
 return (num > a && num < b);
 };
 // store initial columns positions
 cols.each(function() {
 var col = $(this);
 positionsMap[col.attr('id')] = col.position().left;
 });
 // don't bind complex logic directly on scroll –
 // http://ejohn.org/blog/learning-from-twitter/
 content.bind('scroll', function(e) {
 didScroll = true;
 })
 // for each user initiated scroll event 
 // poll current section
 setInterval(function() {
 if ( didScroll && !scriptScroll ) {
 didScroll = false;
 var curScroll = content.scrollLeft(), colID;
 // find what column is selected
 for ( colID in positionsMap ) {
 // safe sex
 if ( {}.hasOwnProperty.call(positionsMap, colID) ) {
 // we compare current left scroll of container element
 // with initial positions of columns
 if ( isInRange(curScroll, positionsMap[colID] - 150, positionsMap[colID] + 410) ) {
 // cut "col-" from column ID
 currentColHash = colID.substring(4);
 // if current col isn't already selected
 if (location.hash.indexOf(currentColHash) == '-1') {
 // indicate user action
 userScroll = true;
 // highlight current column in the address bar
 location.hash = currentColHash;
 }
 }
 }
 }
 }
 }, 250);
 // Controller
 $(window).bind('hashchange', function() {
 var hash = location.hash;
 if (hash != '' && hash != '#') {
 // cut '#' off
 hash = hash.substring(1);
 // don't override user action
 if (!userScroll) {
 // indicate that scroll happens programmatically
 scriptScroll = true;
 // do the scrolling
 content.scrollTo(
 ( content.scrollLeft() + $('#col-' + hash).position().left ) - 20
 , 500, {
 onAfter: function() {
 // done with JavaScript scrolling
 // start polling current section again
 scriptScroll = false;
 }
 });
 }
 userScroll = false;
 } else {
 // support back-button up to empty-hash state
 content.scrollTo(0);
 }
 // on page load, scroll to the proper section
 }).trigger('hashchange');
 // scroll to the next/previous column controls
 // just updating location.hash 
 // controller will take care of taking appropriate action
 $('.column .scroll').bind('click', function(e) {
 e.preventDefault();
 var arrow = $(this);
 // change the column
 changeColumn({
 direction : arrow.hasClass('right') ? 'right' : 'left', 
 currentColumn : arrow.closest('.column')
 });
 });// .scroll bind('click) fn
 // handle left and right arrows
 $(window).bind('keyup', function(e) {
 // 37 – left arrow
 // 39 - right arrow
 if (e.keyCode == 37 || e.keyCode == 39) {
 e.preventDefault();
 changeColumn({
 direction: (e.keyCode == 39) ? 'right' : 'left'
 });
 }
 });// window bind('keyup') fn
 // updates location.hash with slug of the column
 // user wants to view, either by scrolling to it, 
 // either by navigating to it with arrows/header nav
 //
 // @param {Object} options object
 // @option {String} direction – determens change direction
 // @option {jQuery} currentColumn – currently selected column
 function changeColumn(opts) {
 // defaults
 var settings = {
 direction : 'right',
 currentColumn : (function(){
 // calculate current column from hash if it's selected
 var colHash = location.hash.substring(1);
 // if it's not, we suppose we are at first column
 return (colHash.length != 0) ? $('#col-' + colHash) : cols.first();
 })()
 };
 // merge options and defaults
 $.extend(settings, opts);
 // what's the next column?
 var nextColumn = 
 (settings.direction == 'right')
 // scroll right
 ? (settings.currentColumn.next().length != 0) 
 // scroll to the next column
 ? settings.currentColumn.next()
 // scroll to the first column
 : settings.currentColumn.siblings().first()
 // scroll left
 : (settings.currentColumn.prev().length != 0) 
 // scroll to the previous column
 ? settings.currentColumn.prev()
 // scroll to the last column
 : settings.currentColumn.siblings().last();
 // update the hash 
 location.hash = nextColumn.attr('id').substring(4);
 }// fn changeColumn
 } // fn omami.init
};// obj omami

Answer 1

That's a bit to much code for me to go through in one go but I'll take a stab at one part:

// for each user initiated scroll event 
// poll current section
setInterval(function() {
 if ( didScroll && !scriptScroll ) {
 didScroll = false;
 var curScroll = content.scrollLeft(), colID;
 // find what column is selected
 for ( colID in positionsMap ) {
 // safe sex
 if ( {}.hasOwnProperty.call(positionsMap, colID) ) {
 // we compare current left scroll of container element
 // with initial positions of columns
 if ( isInRange(curScroll, positionsMap[colID] - 150, positionsMap[colID] + 410) ) {
 // cut "col-" from column ID
 currentColHash = colID.substring(4);
 // if current col isn't already selected
 if (location.hash.indexOf(currentColHash) == '-1') {
 // indicate user action
 userScroll = true;
 // highlight current column in the address bar
 location.hash = currentColHash;
 }
 }
 }
 }
 }
}, 250);

I'd write like this:

// for each user initiated scroll event 
// poll current section
setInterval(function() {
 if ( !didScroll || scriptScroll ) return;
 didScroll = false;
 var curScroll = content.scrollLeft(), colID;
 // find what column is selected
 for ( colID in positionsMap ) {
 // safe sex
 if ( !{}.hasOwnProperty.call(positionsMap, colID) ) continue;
 // we compare current left scroll of container element
 // with initial positions of columns
 if ( !isInRange(curScroll, positionsMap[colID] - 150, positionsMap[colID] + 410) )
 continue;
 // cut "col-" from column ID
 currentColHash = colID.substring(4);
 // if current col isn't already selected
 if (location.hash.indexOf(currentColHash) !== -1) continue;
 // indicate user action
 userScroll = true;
 // highlight current column in the address bar
 location.hash = currentColHash;
 }
 }
}, 250);

• Use if-guards to save on indentation.

• Use typechecking operators === and !== instead of == and !=. Weird things happen otherwise: http://wtfjs.com/2010/02/26/implicit-tostring-fun, http://wtfjs.com/2011/02/11/all-your-commas-are-belong-to-Array

• str.indexOf returns a number, not a string.

I hope this helped a little. :)

• Misha Reyzlin: Whoa, this if-guards approach was exactly what I was looking for - I felt there is too much indentation going on, but I wasn't sure what to do with it.

Answer 2

I don't have time for a full-blown answer.

Here are only a few pointers:

• Instead of commenting everything, leave only the useful one.

  Sometimes (like in your first setInterval call), it is better to name the function sensibly, instead of commenting on what it does.

• Prefix jQuerified variable names with $. Very helpful

• Do not use more than one ternary (?:) operator at a time. That's just awful.

  Rewrite it as:

  var current = settings.currentColumn,
   next = current[{ right: next, left: prev }[settings.direction]]();
  if (!next.length) { 
   next = current.siblings()[{right: first, left: last}[settings.direction]]
  }
  

  Or better: Add a method to do next with the cycle logic.

I might add more if I do get time to do so

• Misha Reyzlin: Thanks, @glebm! Couple of questions: How to decide when code speaks for itself and when comment's really useful? Is there some kind of rule of thumb? Can you please explain what do you mean exactly by "cycle" logic?
• glebm: Unfortunately, good commenting skills come with experience, and there is no rule of thumb. I usually write the code first, and then comment the parts that I feel might be unclear. By "cycle" logic I meant circular list traversal. You could have an API like current.next({wrap: true}), which would do all the logic of going to the first element if there is no next element.