9
\$\begingroup\$

I've been working with jQuery, mostly consuming plugins and using bits and pieces here and there for other functionality. Anyways, I wrote some standalone functionality in jQuery for a project and decided to convert it to a plugin as an exercise for myself. Note, it's not a full blown plugin, I don't inject markup or CSS at run-time around a specified element; as it stands the plugin code is just acting on markup it expects to be in the specified container in order to work. For now and ease of review.

The plugin source, with full working contrived example, can be accessed at this Fiddle for your review.

Any constructive feedback is welcome, naming conventions, optimizations, general code improvement, do's/don't's, etc...

I've included just the plugin source below, although it is a bit lengthy.

I would appreciate a review of this code.

(function ($) {
$.wwSmoothScroll = function (element, options) {
    var timer; // holds timer for continuous scroll functionality
    var scrollOffset; // how many pixels the content is scrolled on each scroll action
    var continuousScrollSpeed; // affects speed of scrolling when mouse/finger held down on a scroll button

    // settings for auto-scroll feature
    var autoScroll;
    var autoScrollSpeed;
    var autoScrollTimer;
    var autoScrollHandler;
    var autoScrollDirectionChangeDelay;

    var $container;
    var $content;
    var $btnLeftScroll;
    var $btnRightScroll;

    // used to reference the current instance of the object
    var plugin = this;

    var defaults = {
        "scrollOffset": 10,
        "continuousScrollSpeed": 25,
        "autoScroll": false,
        "autoScrollSpeed": 50,
        "autoScrollDirectionChangeDelay": 500
    };

    // used to hold the merged defaults and user provided options
    // within plugin code use: plugin.settings.propertyName
    // outside the plugin use: element.data('pluginName').settings.propertyName, where
    // [element] is the element the plugin has been attached to.
    plugin.settings = {};


    plugin.init = function () {
        // merge default and user-provided settings
        plugin.settings = $.extend({}, defaults, options);

        // store references to various jQuery objects
        $container = $(element);
        $btnLeftScroll = $container.find(".scroller-left.button");
        $btnRightScroll = $container.find(".scroller-right.button");
        $content = $container.find("ul");

        $btnRightScroll.on("mousedown touchstart", function () {
            Scroll(ScrollRight)
        })
            .on("mouseup touchend", ScrollStop);
        $btnLeftScroll.on("mousedown touchstart", function () {
            Scroll(ScrollLeft)
        })
            .on("mouseup touchend", ScrollStop);

        $content.on("mouseenter", Content_OnMouseEnter)
            .on("mouseleave", Content_OnMouseLeave);

        if ($content[0].scrollWidth > $content.outerWidth()) $btnRightScroll.fadeIn("slow");

        if (plugin.settings.autoScroll) {
            autoScrollHandler = ScrollRight;
            autoScrollTimer = setTimeout(RunAutoScroll, plugin.settings.autoScrollSpeed);
        }
    };

    // *** PUBLIC METHODS ***

    plugin.scrollRight = function () {
        ScrollRight();
        return plugin;
    }
    plugin.scrollLeft = function () {
        ScrollLeft();
        return plugin;
    }
    plugin.scrollStop = function () {
        ScrollStop();
        return plugin;
    }
    plugin.exitPlugin = function () {
        return $container;
    }

    // *** PRIVATE METHODS ***

    var RunAutoScroll = function () {
        var isDirectionChanged = false;

        // check for visibility of the main container, don't want this and scroll code executing if the container has been hidden
        if ((plugin.settings.autoScroll) && ($container.is(":visible"))) {
            // if the right button is still visible and the scroll direction is to the right, continue scrolling right
            if ($btnRightScroll.is(":visible") && autoScrollHandler === ScrollRight) {
                // do nothing for now...
            } else {
                var isLeftScrollButtonVisible = $btnLeftScroll.is(":visible");
                if (isLeftScrollButtonVisible && autoScrollHandler === ScrollRight) {
                    isDirectionChanged = true;
                    autoScrollHandler = ScrollLeft;
                } else if (!isLeftScrollButtonVisible && autoScrollHandler === ScrollLeft) {
                    isDirectionChanged = true;
                    autoScrollHandler = ScrollRight;
                }
            }

            // if direction has been swapped, wait the defined period of time before actually scrolling
            if (isDirectionChanged) autoScrollTimer = setTimeout(RunAutoScroll, plugin.settings.autoScrollDirectionChangeDelay);
            else {
                autoScrollHandler();
                autoScrollTimer = setTimeout(RunAutoScroll, plugin.settings.autoScrollSpeed);
            }
        }
    };

    var Content_OnMouseEnter = function () {
        if (plugin.settings.autoScroll) clearTimeout(autoScrollTimer);
    };

    var Content_OnMouseLeave = function () {
        if (plugin.settings.autoScroll) autoScrollTimer = setTimeout(RunAutoScroll, plugin.settings.autoScrollSpeed);
    };

    var Scroll = function (scrollHandler) {
        if (scrollHandler !== undefined) {
            /* disable autoscroll once the user has clicked on a scroll button */
            clearTimeout(autoScrollTimer);
            plugin.settings.autoScroll = false;
            scrollHandler();
            timer = setInterval(scrollHandler, plugin.settings.continuousScrollSpeed);
        }
    };

    var ScrollRight = function () {
        console.log("ScrollRight()");
        var maxScrollOffsetLeft = $content[0].scrollWidth - $content.outerWidth();
        if ($content.scrollLeft() < maxScrollOffsetLeft) {
            $content.scrollLeft($content.scrollLeft() + plugin.settings.scrollOffset);
            $btnLeftScroll.fadeIn("slow");
        } else {
            if (timer !== undefined) clearInterval(timer);
            $btnRightScroll.fadeOut("slow");
        }
    };

    var ScrollLeft = function () {
        console.log("ScrollLeft()");
        if ($content.scrollLeft() > 0) {
            $content.scrollLeft($content.scrollLeft() - plugin.settings.scrollOffset);
            $btnRightScroll.fadeIn("slow");
        } else {
            if (timer !== undefined) clearInterval(timer);
            $btnLeftScroll.fadeOut("slow");
        }
    };

    var ScrollStop = function () {
        clearInterval(timer);
    };

    // Call the "constructor" for the plugin
    plugin.init();
};

// add the plugin to the jQuery.fn object
$.fn.wwSmoothScroll = function (options) {
    return this.each(function () {
        if (undefined == $(this).data("wwSmoothScroll")) {
            // create a new instance of the plugin
            var plugin = new $.wwSmoothScroll(this, options);
            // store a reference to the plugin object
            $(this).data("wwSmoothScroll", plugin);
        }
    });
};
})(jQuery);

UPDATE: I changed the jQuery to 1.9 in the Fiddle as under IE10 it wasn't working. Wasn't using anything specific to jQuery 2.x

UPDATE: Changed the defaults variable to use string literals to avoid collisions.

\$\endgroup\$
7
\$\begingroup\$

Overal, I think this is very good code, I only have a few pointers.

  • Unused variables ( jshint ).

    • scrollOffset
    • continuousScrollSpeed
    • autoScroll
    • autoScrollSpeed
    • autoScrollDirectionChangeDelay
  • Yoda conditions :

    • (undefined == $(this).data("wwSmoothScroll"))
    • (scrollHandler !== undefined)
    • Either have all Yoda conditions or no Yoda conditions, also compare to undefined with ===. Or use/try falsey conditions.
  • Chaining event handlers, it looks ugly. The problem is that I dont like what beautifiers do either. I counter propose the following formatting, which I can grok in a split second, feel free to ignore:

$btnRightScroll.on("mousedown touchstart", function () { Scroll(ScrollRight) })
               .on("mouseup touchend", ScrollStop);

$btnLeftScroll.on("mousedown touchstart", function () { Scroll(ScrollLeft) })
              .on("mouseup touchend", ScrollStop);

$content.on("mouseenter", Content_OnMouseEnter)
        .on("mouseleave", Content_OnMouseLeave);
  • Finally, it is considered best practice to have one chained var statement, so
var timer, // holds timer for continuous scroll functionality
    scrollOffset, // how many pixels the content is scrolled on each scroll action
    continuousScrollSpeed, // affects speed of scrolling when mouse/finger held down on a scroll button

    // settings for auto-scroll feature
    autoScroll,
    autoScrollSpeed,
    etc.
\$\endgroup\$
  • 2
    \$\begingroup\$ Some comments: Unused Variables: whoops! I changed the implementation and neglected the clean-up (not good practice). YODA: Thank you for pointing those out. Formatting: Thank you for the suggestions, and I prefer the formatting you provided. Chained VAR: I've been away from JavaScript for a while, thank you for the best practice pointer. Again, thank you for taking the time to review! \$\endgroup\$ – emgee Dec 23 '13 at 19:02

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.