7
\$\begingroup\$

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
  };
})();
\$\endgroup\$
1
\$\begingroup\$

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 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 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];
}
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$

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.