# JavaScript accordion

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();  ## 1 Answer 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?

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.
• You could simply do \$groupSiblings.find('li.is-expanded').removeClass('is-expanded') instead of that if (and var) :) – ᔕᖺᘎᕊ Aug 20 '15 at 13:09