patternjavascriptMinor
Setting currency and language values
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 (
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
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
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:
should be
can be replaced with
it will not add the class multiple times.
- 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.