STStratoscope·Mar 18, 2013
I opened this page:
https://github.com/shichuan/javascript-patterns/blob/master/...
and saw this pattern used several times:
if (value == 0) {
return result0;
} else if (value == 1) {
return result1;
} else if (value == 2) {
return result2;
}
I can't think of a coding standard or best practice in any language that would suggest combining return and else like that. Instead it would be:
if (value == 0) {
return result0;
}
if (value == 1) {
return result1;
}
if (value == 2) {
return result2;
}
or:
if (value == 0) return result0;
if (value == 1) return result1;
if (value == 2) return result2;
Then on this page:
https://github.com/shichuan/javascript-patterns/blob/master/...
there is this code:
// optimization 1 - cache the length of the
Read the rest
array with the use of `max`
for (var i = 0, max = myarray.length; i < max; i++) {
// do something with myarray[i]
}
The code is OK, but 'max' is a very poor name. It sounds too much like Math.max, and it isn't the maximum value for the loop. It's one greater than that.
If you want to cache the array length like this, then any of these names would be far better than max: either 'length' to match the 'array.length', or 'len' for something shorter, or 'n' if you want it really short. These names all better reflect the meaning of this variable: the length of the array. ('n' stands for 'number', as in the number of elements in the array.)
Later on the page it recommends:
// preferred 1
var i, myarray = [];
for (i = myarray.length; i--;) {
// do something with myarray[i]
}
I'm not a fan of running loops backwards for optimization except where it's really necessary. When I'm reading the code I rarely know whether the backwards loop is simply an optimization, or essential to the algorithm. It adds mystery to the code, and I don't like that.
Of course a comment could be added saying "Backwards loop is for optimization only" or "Backwards loop is needed for the algorithm", but I've never seen a backwards loop aficionado do this.
A few years ago I worked with someone who hated jQuery and libraries in general and preferred to write his own bare metal DOM loops for everything. And he wrote them all backwards. Later on he warmed up a bit to the idea of using jQuery, after seeing a 20-line bare metal function be reduced to a simple and straightforward selector, but when we started going through the code it was never clear if we needed to add a ":last" to the selector to make it match the backwards loop or not.
It's pretty rare that this kind of loop optimization will make much difference, unless the work done in the loop body is truly minimal.
Then on this page:
https://github.com/shichuan/javascript-patterns/blob/master/...
it has this code:
// somewhere else in the code
// a method was added to all objects
if (typeof Object.prototype.clone === 'undefined') {
Object.prototype.clone = function () {
};
}
Now this is part of a discussion about why you should use 'hasOwnProperty()' in a for-in loop to avoid enumerating Object.prototype additions, but there should also be a mention that you should not extend Object.prototype at all, ever, unless you are 100% certain that all the code in your page or project is under your own control and carefully uses hasOwnProperty().
For example, the Google Maps API has for-in loops that do not use hasOwnProperty(). If you extend Object.prototype, then no matter how careful you are in the rest of your own code, you'll have some confusing problems if you later want to add a Google map to your page.
(At least the Maps API used to work this way - it's possible that they have started using hasOwnProperty() now - I haven't checked it in a while. But the same problem could occur with any third-party code you may want to use.)
OK, well, on this page:
https://github.com/shichuan/javascript-patterns/blob/master/...
it does mention that you shouldn't extend Object.prototype, but there's no hint as to why you shouldn't.
On this page:
https://github.com/shichuan/javascript-patterns/blob/master/...
switch (inspect_me) {
case 0:
result = "zero";
break;
case 1:
result = "one";
break;
default:
result = "unknown";
}
Omitting the break; on the last case in a switch statement is a bad idea. It's too easy to add another case below that and forget the break statement. Better to use break on every case even if it is redundant at the end.