I been getting into javascript patterns and would love to get some feedback. I notice that I got some general helper functions. Is this a good approach of dealing with it?
// carousel.js
var Carousel = (function() {
var timeout = 340;
function init() {
applyForTheseQueries();
$(window).on('resize', Helpers.debounce(function() {
applyForTheseQueries();
}, timeout));
}
function applyForTheseQueries() {
var $carousel = $('.js-slick');
var query = $carousel.data('query');
var mediaQuery = Helpers.executeFunctionByName(query, window);
if (mediaQuery && !$carousel.hasClass('init-done')) {
$carousel.addClass('init-done').slick();
} else if (!mediaQuery && $carousel.hasClass('init-done')) {
$carousel.removeClass('init-done').slick('unslick');
}
}
return {
init: init
};
})();
$(function() {
Carousel.init();
});
// helpers.js
var Helpers = (function() {
function executeFunctionByName(functionName, context) {
var args = [].slice.call(arguments).splice(2);
var namespaces = functionName.split(".");
var func = namespaces.pop();
for (var i = 0; i < namespaces.length; i++) {
context = context[namespaces[i]];
}
return context[func].apply(context, args);
}
function debounce(func, wait, immediate) {
var timeout;
return function() {
var context = this;
var args = arguments;
var later = function() {
timeout = null;
if (!immediate) func.apply(context, args);
};
var callNow = immediate && !timeout;
clearTimeout(timeout);
timeout = setTimeout(later, wait);
if (callNow) func.apply(context, args);
};
};
return {
executeFunctionByName: executeFunctionByName,
debounce: debounce
};
})();
2 Answers 2
This approach seems fine. I did notice the following loop in Helpers. executeFunctionByName()
:
for (var i = 0; i < namespaces.length; i++) { context = context[namespaces[i]]; } return context[func].apply(context, args);
Doesn't that just set context
equal to the last element in namespaces
? Why not just use a simple assignment without a loop?
The value for the timeout
var timeout = 340;
Could be declared with all capitals as is idiomatic in JavaScript and many C-based languages, as well as a more descriptive name:
const RESIZE_TIMEOUT = 340;
There are a few things that could be simplified.
In Carousel.init
the resize callback could be simplified from
$(window).on('resize', Helpers.debounce(function() { applyForTheseQueries(); }, timeout));
To this:
$(window).on('resize', Helpers.debounce(applyForTheseQueries, timeout));
And if ecmascript-6 syntax is supported, then object initializer shorthand notation can be used - instead of :
return { init: init }
just use:
return {
init
}
And instead of
return { executeFunctionByName: executeFunctionByName, debounce: debounce };
Simply to
return {
executeFunctionByName,
debounce
};
Another ecmascript-6 feature that could be used is the for...of
loop. Instead of
for (var i = 0; i < namespaces.length; i++) { context = context[namespaces[i]]; }
Simplify this to
for (const namespace of namespaces) {
context = context[namespace];
}
Yes, this is a nice, clean way to do it. I do not see smth wrong here. You have your helper methods isolated, they are exposed correctly, and the methods within are DRY, they only care for what they get from arguments, and they return one thing only.
Maybe you have to get sure that nothing breaks if init()
gets called for more than one times, that is the one thing I would suggest.
Explore related questions
See similar questions with these tags.