1
\$\begingroup\$

I've just created a method that toggles menu visibility. At the moment I've gone with an if..else which checks my flag and acts appropriately. Can anyone offer any suggestions on how to make this snippet better? Maybe it shouldn't all live in the same snippet? Maybe I should be combining the show/hide logic that's duplicated in the if..else?

onNavTriggerClick: function(event) {

            var $currentTarget = $(event.currentTarget);

            if(this._navOpen) {
                Backbone.Events.trigger('hide:menu');
                $currentTarget.removeClass('is-active');
                this._navOpen = false;
            } else {
                Backbone.Events.trigger('show:menu');
                $currentTarget.addClass('is-active');
                this._navOpen = true;
            }
        }
\$\endgroup\$
1
\$\begingroup\$

You already mentioned the duplication, so let's address that issue.

First, I'd move the assignment to a common block so it's clear that you're toggling the visibility.

Then for toggling the CSS class I found the toggleClass function, so maybe go with that.

After that moving the assignment to the top, then working based on the new state gives me this:

onNavTriggerClick: function(event) {
  this._navOpen = !this._navOpen;
  Backbone.Events.trigger(this._navOpen ? 'show:menu' : 'hide:menu');
  $(event.currentTarget).toggleClass('is-active', this._navOpen);
}

Looks a bit more concise.

\$\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.