jQuery dropdown menu

Question

I wrote this jQuery code as I could for a dropdown menu.

But I'm sure there is a way to make this code shorter.

$(document).ready(function() {
 $('#show-menu-1').on('click touch', openSubMenu);
 $('#close-menu-1').on('click touch', closeSubMenu);
 $(document).on('click touch', function(event) {
 if (!$(event.target).closest('#menu-1').length) {
 $('#menu-1 > .main-menu__submenu').removeClass('main-menu__submenu--active');
 $('#menu-1').removeClass('main-menu__item--active');
 }
 });
 function openSubMenu() {
 $('#menu-1 > .main-menu__submenu').addClass('main-menu__submenu--active');
 $('#menu-1').addClass('main-menu__item--active');
 };
 function closeSubMenu() {
 $('#menu-1 > .main-menu__submenu').removeClass('main-menu__submenu--active');
 $('#menu-1').removeClass('main-menu__item--active');
 };
});
$(document).ready(function() {
 $('#show-menu-2').on('click touch', openSubMenu);
 $('#close-menu-2').on('click touch', closeSubMenu);
 $(document).on('click touch', function(event) {
 if (!$(event.target).closest('#menu-2').length) {
 $('#menu-2 > .main-menu__submenu').removeClass('main-menu__submenu--active');
 $('#menu-2').removeClass('main-menu__item--active');
 }
 });
 function openSubMenu() {
 $('#menu-2 > .main-menu__submenu').addClass('main-menu__submenu--active');
 $('#menu-2').addClass('main-menu__item--active');
 };
 function closeSubMenu() {
 $('#menu-2 > .main-menu__submenu').removeClass('main-menu__submenu--active');
 $('#menu-2').removeClass('main-menu__item--active');
 };
});

• 200_success: We could probably do a better job of reviewing your code if you also included the corresponding HTML. You can use Ctrl-M in the question editor to include it.

Answer 1

You can make this shorter by modifying your html to have the id in a data attribute and using classes, but this is what I have based on the info you've given.

$(function() {
 function initializeMenu(id) {
 $('#show-menu-' + id).on('click touch', function() { toggleMenu(id, true); });
 $('#close-menu-' + id).on('click touch', function() { toggleMenu(id, false); });
 $(document).on('click touch', function(event) {
 if (!$(event.target).closest('#menu-' + id).length) {
 toggleMenu(id, false);
 }
 });
 }
 function toggleMenu(id, open) {
 $('#menu-' + id + ' > .main-menu__submenu').toggleClass('main-menu__submenu--active', open);
 $('#menu-' + id).toggleClass('main-menu__item--active', open);
 }
 for(var i = 1; i <= 2; i++) {
 initializeMenu(i);
 }
});

Answer 2

Functions

Make "#show-menu-2" and "#close-menu-2" parameters and turn the contents of your $(document).ready() into it's own function so that you may call it again. That'll instantly reduce the code in half. Then you'd just call thatFunction("#show-menu-2", "#close-menu-2") and thatFunction("#show-menu-1", "#close-menu-1").

Redundancy

Your click touch event is calling code identical to your closeSubMenu function. Just call closeSubMenu();. Parameterizing it in a similar fashion to the above example will allow you to do so.

• AD7six: having function initializeMenu(whichOne) { ... } initializeMenu(1); initializeMenu(2); would make more sense, only the numerical suffix appears to differ between the two ready handlers. I think it would be more useful to show the code changes you are suggesting, rather than describe them.
• Shelby115: I'm a fan of flexibility when it comes to functions. So I lean towards suggesting the whole selector be the parameter so that you may reuse this function elsewhere. Not necessarily in the same project with the same styles. Might be overkill for this situation though, not sure.
• AD7six: There's flexiblity and then there's YAGNI ;).