5
\$\begingroup\$

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
\$\endgroup\$
4
\$\begingroup\$

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

I hope this helped a little. :)

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Misha Reyzlin Mar 13 '11 at 11:01
2
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Misha Reyzlin Mar 13 '11 at 10:11
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – glebm Mar 13 '11 at 13:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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