3 ms·
I'm not the most savvy javascript developer, but I'll take a stab at it. Variable declarations and assignments happen on multiple lines in the 2nd snippet inst
by WesleyJohnson 15y ago
I'm not the most savvy javascript developer, but I'll take a stab at it.
Variable declarations and assignments happen on multiple lines in the 2nd snippet instead of all being defined at once.
var currentLanguage = "en";
var spanish = "es";
var german = "de";
var english = "en";
vs
var currentLanguage = "en", spanish = "es", german = "de", english = "en";
Additionally, "en" is assigned twice. If you're going to take the time to assign the "en" abbreviation into the "english" variable, then you should use it when assigning to "currentLanguage", in my opinion, if only to avoid typos and redundancy.
var spanish = "es", german = "de", english = "en", currentLanguage = english;
The "disclaimer" element is accessed using the documentGetElementById DOM call twice. javascript is getting faster, but forcing multiple DOM calls when it's not necessary is bad practice. Ideally, you'd access it once and save it into a reference.
document.getElementById("disclaimer").firstChild.nodeValue = response;
var text = encodeURIComponent(document.getElementById("disclaimer").firstChild.nodeValue);
vs
var disclaimer = document.getElementById("disclaimer"), text = disclaimer.firstChild.nodeValue;
disclaimer.firstChild.nodeValue = response;
The code formatting is poor and inconsistent, notice an incorrect space after "encodeURIComponent (", but no spaces after "getElementById(" and "getElementsByTagName(". Also notice no indentation anywhere, or line-breaks after the function signatures and opening brackets "{";
The code is polluting the global namespace by not being enclosed in its own namespace or perhaps an immediately executed anonymous function.
I'm sure more seasoned JS devs can list some more things wrong with the code, but those are the ones that stood out to me.
- ward 15y agoI don't see a bunch of variable definitions on one line as an improvement, just reads less easily and only feels justified in the case of some low numbers like var x = 0, y = 1, z = 0; // Some looping or similar here I do agree with your using of `english` variable to assign `currentLanguage`.
- WesleyJohnson 15y agoPersonally, I like doing all the declarations on their own line as well. But anytime I've used JSLint in the past, I seem to recall it complaining about that. Perhaps I'm mistaken?
- masklinn 15y agoI expect JSLint was whining about using multiple `var` statements. Using a single var and putting each declaration on its line is not incompatible: var foo = 4, bar = 5, baz = 6, qux = 'quux;
- ward 15y agoThat feels like a situation that could easily end up (copy pasting something, someone adding another var, ...) in an unexpected global variable declaration. var foo = 4, bar = 5, baz = 6, sprinkles = 'wee'; qux = 'quux'; Making `qux` global.
- mattmanser 15y agoA lot of developers don't like declaring more than one variable on one line, that's a style choice. There's nothing wrong with it, especially in JS where accidentally deleting a variable declaration can suddenly turn your variable global. The problem with the code is more that the code stinks, even after you remove the formatting problems, it looks more like .Net code and it doesn't make any sense.