Setting currency and language values

Question

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:

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.
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();
});

konijn · Accepted Answer · 2013-08-29 19:25:16Z

7

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.

edited Aug 29, 2013 at 19:25

answered Aug 29, 2013 at 15:33

konijn

34.4k5 gold badges71 silver badges267 bronze badges

\$\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
– Brian

2013-08-29 16:43:57 +00:00
Commented Aug 29, 2013 at 16:43
1

\$\begingroup\$ I actually wrote exactly that and then removed it. \$\endgroup\$

konijn
– konijn

2013-08-29 18:03:57 +00:00
Commented Aug 29, 2013 at 18:03
\$\begingroup\$ Thank you, I've applied your suggestions to my code (I've edited my original post) \$\endgroup\$

user1732521
– user1732521

2013-08-29 18:47:03 +00:00
Commented Aug 29, 2013 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
– Dave Jarvis

2013-08-29 19:59:34 +00:00
Commented Aug 29, 2013 at 19:59
\$\begingroup\$ @DaveJarvis Sure, but for JavaScript the standard is camelCase, when in Rome.. \$\endgroup\$

konijn
– konijn

2013-08-29 20:21:57 +00:00
Commented Aug 29, 2013 at 20:21

Add a comment |

Changaco · Accepted Answer · 2013-08-29 19:09:27Z

3

.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();

answered Aug 29, 2013 at 19:09

Changaco

4412 silver badges6 bronze badges

\$\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
– user1732521

2013-08-30 13:58:24 +00:00
Commented Aug 30, 2013 at 13:58

Add a comment |

Stack Exchange Network

Setting currency and language values

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Setting currency and language values

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions