3
\$\begingroup\$

I'd like to improve my code, but I'm not sure where to start. Everything works as I want it to; I just think it's ugly.

I'll split this into two parts:

  1. I have two drop-downs that I'll explain later. In these drop-downs, I have options to set currency and language. I use a hidden form to set the current values with PHP, and extract the current value from these field. I then compare if the selected value matches the initial one.

  2. Within my two drop-downs, clicking on one or the other closes the other one, and open itself if it's not open. Upon closing the drop-down, I check if I should submit the form (reloadPage()).

I think this should be split into separate functions, but I don't really know how to merge all of this together. I'm not asking for a full recode of my stuff, but I'd appreciate how to make this less ugly.

Part 1

$(function() {
    hasChanged = false;
    var $options = $('#options');
    var $currencycode = $options.find('#currency_code');
    var $languagecode = $options.find('#language_code');
    var $initCurrencycode = $currencycode.val();
    console.log($initCurrencycode);
    $('.monnaie').find('a').click(function() {
        $this = $(this);
        if(!$this.hasClass('selected')) {
            $this.parent().siblings().find('a').removeClass('selected');
            $myCurrencycode = $this.attr('class');
            $currencycode.val($myCurrencycode);
            $this.addClass('selected');
            if($myCurrencycode == $initCurrencycode) {
                hasChanged = false;
                console.log(hasChanged);
            } else {
                hasChanged = true;
                console.log(hasChanged);
            }
        }
    });
});

Part 2

$optionsdropdown = $('#mobile-options');
$menudropdown = $('#mobile-menu');
function reloadPage() {
    if(hasChanged) {
        alert('reload now');
    } else {
        console.log('nope');
    }
}
$('.options-toggler').click( function() {
    if(!$optionsdropdown.hasClass('hide')) {
        $optionsdropdown.addClass('hide');
    } else {
        $optionsdropdown.removeClass('hide');
    }
    if(!$menudropdown.hasClass('hide')) {
        $menudropdown.addClass('hide');
    }
});
$('.menu-toggler').click( function() {
    if(!$menudropdown.hasClass('hide')) {
        $menudropdown.addClass('hide');
    } else {
        $menudropdown.removeClass('hide');
    }
    if(!$optionsdropdown.hasClass('hide')) {
        $optionsdropdown.addClass('hide');
    }
});
$('html').click(function() {
    if(!$optionsdropdown.hasClass('hide')) {
        $optionsdropdown.addClass('hide');
    }
    if(!$menudropdown.hasClass('hide')) {
        $menudropdown.addClass('hide');
        reloadPage();
    }
});
$('#mobile-menu, .menu-toggler').click(function(event){
    event.stopPropagation();
});
$('#mobile-options, .options-toggler').click(function(event){
    event.stopPropagation();
});
\$\endgroup\$
7
\$\begingroup\$

My 2 cents:

  • Use jQuery toggleClass : http://api.jquery.com/toggleClass/
  • Use camelCase : currencycode -> currencyCode etc.
  • Use English everywhere : 'monnaie' should not be there
  • Booleans are awesome
        if($myCurrencycode == $initCurrencycode) {
            hasChanged = false;
            console.log(hasChanged);
        } else {
            hasChanged = true;
            console.log(hasChanged);
        }

should be

   hasChanged = ($myCurrencycode != $initCurrencycode)
   console.log( hasChanged );
  • Finally,
if(!$menuDropdown.hasClass('hide')) {
  $menuDropdown.addClass('hide');
}

can be replaced with $menuDropdown.addClass('hide'); it will not add the class multiple times.

\$\endgroup\$
  • \$\begingroup\$ OP isn't even using hasChanged, so he could also go a step further and use console.log($myCurrencycode != $initCurrencycode);. Though that's starting to approach the edge between brevity and readability. \$\endgroup\$ – Brian Aug 29 '13 at 16:43
  • 1
    \$\begingroup\$ I actually wrote exactly that and then removed it. \$\endgroup\$ – konijn Aug 29 '13 at 18:03
  • \$\begingroup\$ Thank you, I've applied your suggestions to my code (I've edited my original post) \$\endgroup\$ – user1732521 Aug 29 '13 at 18:47
  • \$\begingroup\$ @tomdemuyt: "results indicate a significant improvement in time and lower visual effort with the underscore style" ~ cs.kent.edu/~jmaletic/papers/… \$\endgroup\$ – Dave Jarvis Aug 29 '13 at 19:59
  • \$\begingroup\$ @DaveJarvis Sure, but for JavaScript the standard is camelCase, when in Rome.. \$\endgroup\$ – konijn Aug 29 '13 at 20:21
3
\$\begingroup\$

.addClass() won't add a class multiple times, so this:

if(!$optionsDropdown.hasClass('hide')) {
    $optionsDropdown.addClass('hide');
}

can be rewritten as:

$optionsDropdown.addClass('hide');

But why even use a hide class at all ? jQuery has .hide() and .show() helper functions, so you can just write:

$optionsDropdown.hide();
\$\endgroup\$
  • \$\begingroup\$ Because I'm applying the class in the source, I find it more semantic to hide a dropdown menu with a class than with hide(). I might be wrong, but I don't know. I will update my code to use addClass once, thanks :) \$\endgroup\$ – user1732521 Aug 30 '13 at 13:58

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.