7
\$\begingroup\$

I have written a jQuery plugin (my first for a long time) that simply cycles through strings of text in a data attribute and uses setInterval to type it out - it's a common effect you've probably seen hundreds of times.

I'm looking for a code review of the JavaScript/jQuery portion of the code mainly because a lot of the literature on building a jQuery plugin is quite old and I want to be sure I'm doing things the "modern" way.

CodePen

$(document).ready(function() {
  $().typeText(); 
});

(function($) {

  $.fn.typeText = function(options) {

    var settings = $.extend({
      loopSpeed: 5000,
      typeSpeed: 100,
      selector: '.words'
    }, options);

    var typeIntervals = [];

    load();

    function load() {

      $(settings.selector).each(function() {
        var wordsArray = $(this).data('words').split(',');
        var length = wordsArray.length - 1;
        var index = 0;
        var thisWordGroup = $(this);;
        if (typeof $(this).data('index') == 'undefined') {
          $(thisWordGroup).data('index', 0);
        }
        $(this).html(wordsArray[0]);
      });

      setIntervals();

    }

    function setIntervals() {

      setInterval(function() {
        clearTypeIntervals();
        $(settings.selector).each(function() {
          var thisWordGroup = $(this);
          var wordsArray = thisWordGroup.data('words').split(',');
          var length = wordsArray.length - 1;
          var index = thisWordGroup.data('index');
          index++;
          if (index > length) {
            index = 0;
          }
          thisWordGroup.data('index', index);
          type(thisWordGroup, wordsArray[index]);
        });
      }, settings.loopSpeed);

    }

    function type($selector, word) {
      $selector.html('');
      var index = 0;
      var splitWord = word.split('');
      var splitWordLength = splitWord.length - 1;
      var intervalName = 'typeWordInterval' + Math.ceil(Math.random() * 99999);
      typeIntervals[intervalName] = setInterval(function() {
        var currentText = $selector.html();
        $selector.html(currentText + splitWord[index]);
        if (index < splitWordLength) {
          index++
        } else {
          clearInterval(typeIntervals[intervalName])
        }
      }, settings.typeSpeed);
    }

    function clearTypeIntervals() {
      for (var element in typeIntervals) {
        clearInterval(typeIntervals[element]);
      }
    }

  }

}(jQuery));
\$\endgroup\$
7
\$\begingroup\$

The API design is rather weird. You expect your plugin to be called like

$().typeText({ selector: '.words' })

But a design like this would be much more natural for jQuery:

$('.words').typeText()
\$\endgroup\$
  • \$\begingroup\$ Good point, yes I should do that. Thanks \$\endgroup\$ – CJD Sep 13 '15 at 20:08
4
\$\begingroup\$
  $(settings.selector).each(function() {
    var wordsArray = $(this).data('words').split(',');
    var length = wordsArray.length - 1;
    var index = 0;
    var thisWordGroup = $(this);;
    if (typeof $(this).data('index') == 'undefined') {
      $(thisWordGroup).data('index', 0);
    }
    $(this).html(wordsArray[0]);
  });
  1. The variables length and index are unused, so you can just delete them.

  2. There is no need to do all this typeof checking with "undefined"; if the data value you entered does not exist, it will return undefined, which will result as false in an if conditional.


function setIntervals() {

  setInterval(function() {
    clearTypeIntervals();
    $(settings.selector).each(function() {
      var thisWordGroup = $(this);
      var wordsArray = thisWordGroup.data('words').split(',');
      var length = wordsArray.length - 1;
      var index = thisWordGroup.data('index');
      index++;
      if (index > length) {
        index = 0;
      }
      thisWordGroup.data('index', index);
      type(thisWordGroup, wordsArray[index]);
    });
  }, settings.loopSpeed);

}
  1. This small construct:
var index = thisWordGroup.data("index");
index++;
if (index > length) {
  index = 0;
}

This, with the incrementing in the middle along with the conditional makes this area a little clunky. These lines can be easily reduces using a ternary operator:

var index = thisWordGroup.data("index")++;
index = index > length ? 0 : index;
  1. The incrementing ++ was moved to the same line that index was being stored. This looks more clean now, in my opinion.

  2. The if conditional was converted into a ternary operator to set a new value to index. This also makes the code look more clean, in my opinion.


function clearTypeIntervals() {
  for (var element in typeIntervals) {
    clearInterval(typeIntervals[element]);
  }
}
  1. In JavaScript, it is bad practice to iterate through an array using a for/in loop. You should be using a normal for loop:


for(var i = 0, length = typeIntervals.length; i < length; i++) {
    clearInterval(typeIntervals[i]);
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed review! Makes sense with removing typeof and agree with you about improving the readability for the index incrementer. Out of interest though, why is 'for in' bad practice? \$\endgroup\$ – CJD Sep 13 '15 at 20:12
  • \$\begingroup\$ @CJD I don't know specifically why it is bad practice, but my thoughts are that for/in loops are meant for iterating through the keys of an object. And, yes, while an array is technically an object in JavaScript, I've been taught that it is better practice to use a normal for loop. \$\endgroup\$ – SirPython Sep 13 '15 at 20:14

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.