1
\$\begingroup\$

I'm trying to refactor some code I wrote months ago when I used to use lots of anonymous functions. The way I'm trying to do it now is by taking all those functions and rewrite them within Object Literal Pattern.

I'm new to this kind of pattern and I would like to know if I'm doing it right, what should I improve or if there's a better way to write the code for easier maintainability.

The first piece of code I refactored is in the following rows and is about an accordion view.

I've also created a JsFiddle for this where you can see the whole code including the former way of writing the js.

var mainAccordion = {
  init: function() {
    this.cacheDom();
    this.bindEvents();
  },

  cacheDom: function() {
    this.$elTrigger = $('.js-accordion-trigger');
  },

  bindEvents: function() {
    this.$elTrigger.on('click', this.elExpand);
  },

  elExpand: function(event) {
    var $el = $(this),
        $elContent = $el.next('.accordion__content'),
        $elParent = $el.closest('li'),
        $elSiblings = $el.parent().siblings(),
        $groupSiblings = $el.parents('.list-with-header').siblings();

    if (!$elParent.hasClass('is-expanded')) {
      event.preventDefault();

      $elContent.show();
      $elParent.addClass('is-expanded');

      $elSiblings.removeClass('is-expanded');
      $elSiblings.find('.accordion__content').hide();

      if ($groupSiblings.find('li').hasClass('is-expanded')) {
        $groupSiblings.find('li').removeClass('is-expanded');
        $groupSiblings.find('.accordion__content').hide();
      }

      // animate accordion element to top of page
      $('html, body').stop().animate({
        'scrollTop': $elParent.offset().top
      });
    } else {
      event.preventDefault();

      $elContent.slideUp('fast');
      $elParent.removeClass('is-expanded');
    }
  }
};

mainAccordion.init();
\$\endgroup\$
2
\$\begingroup\$

Some small changes I'd recommend listed below:

D.R.Y.

Why don't you call event.preventDefault before your control statement since it will be called in both the if and the else case?

Avoid doubling up on lookups

I am not very experienced in jQuery, but can't you make some if checks not double up on lookups?

var liGroup = $groupSiblings.find('li')
if (liGroup.hasClass('is-expanded')) {
     liGroup.removeClass('is-expanded');

Also check the specs on removeClass()..perhaps you can call removeClass() without checking hasClass?

Formatting/Readability

Your code is difficult to read without more formatting. For example, it's not clear which if matches up with the else from skimming the code. It shouldn't be necessary for a reader to parse parens/brackets to figure out what the if...else pairing is.

Also a few more comments would be helpful. For example, comments describing your control statement branching in a few words would be welcome.

\$\endgroup\$
  • 1
    \$\begingroup\$ You could simply do $groupSiblings.find('li.is-expanded').removeClass('is-expanded') instead of that if (and var) :) \$\endgroup\$ – ᔕᖺᘎᕊ Aug 20 '15 at 13:09
  • \$\begingroup\$ @ᔕᖺᘎᕊ thanks for confirming, had just added an edit to my own answer asking about that. \$\endgroup\$ – sunny Aug 20 '15 at 13:15

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.