HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

First jQuery Plugin - SmoothSlider

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
smoothsliderpluginfirstjquery

Problem

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 a

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#29288, answer score: 7

Revisions (0)

No revisions yet.