HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Setting currency and language values

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
valueslanguagesettingandcurrency

Problem

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(!$optionsdropdow

Solution

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.

Code Snippets

if($myCurrencycode == $initCurrencycode) {
            hasChanged = false;
            console.log(hasChanged);
        } else {
            hasChanged = true;
            console.log(hasChanged);
        }
hasChanged = ($myCurrencycode != $initCurrencycode)
   console.log( hasChanged );
if(!$menuDropdown.hasClass('hide')) {
  $menuDropdown.addClass('hide');
}

Context

StackExchange Code Review Q#30441, answer score: 7

Revisions (0)

No revisions yet.