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));
200_success
146k22 gold badges190 silver badges479 bronze badges
asked Sep 13, 2015 at 16:12
\$\endgroup\$

2 Answers 2

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()
answered Sep 13, 2015 at 19:20
\$\endgroup\$
0
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]);
}
answered Sep 13, 2015 at 19:47
\$\endgroup\$
2
  • \$\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\$ Commented Sep 13, 2015 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\$ Commented Sep 13, 2015 at 20:14

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.