Lets share some Javascript ES5 coding mistakes you know

[DoubleX]

This topic aims to incite you to share as many javascript ES5 coding mistakes as you know, in order to help all of us learn from those shared mistakes, or simply make fun of them

Let's start by sharing mine:

Not knowing Javascript ES5 variables are references

Spoiler

Example - Treating this:

// array1 and array2 refers to 2 different empty arraysarray1 = [];array2 = [];As the same as this:

array1 = array2 = []; // Both array1 and array2 refer to the same empty arrayHowever, this:

// Both array2 and array3 refer to what array1 currently refers toarray2 = array1;array3 = array1;Is the same as this:

array2 = array3 = array1; // Both array2 and array3 refer to what array1 currently refers toApplication - Thinking that this method's referentially transparent:

// This function actually isn't side effect freesideEffectFreeFunction = function(array, val) var tempArray = array; // The passed argument array will be modified due to sort tempArray.sort(function(a, { return a.someProp - b.someProp; }); for (var index = 0, length = tempArray.length; index < length; index++) { if (tempArray[index].someProp > val) { return tempArray[index]; } } return null;} // sideEffectFreeFunctionWhen a truly side effect free version should be something this :

// This function's really side effect freesideEffectFreeFunction = function(array, val) // tempArray is a new array instead of referring to what array currently refers to var tempArray = array.slice(0); // tempArray.sort(function(a, { return a.someProp - b.someProp; }); for (var index = 0, length = tempArray.length; index < length; index++) { if (tempArray[index].someProp > val) { return tempArray[index]; } } return null;} // sideEffectFreeFunction

Not knowing Javascript ES5 uses short circuit evaluation

Spoiler

Example - Treating this:

// expr1 will always be run but expr2 will only be run if expr1 is falsyif (expr1 || expr2) { expr3; }//As the same as this:

// expr2 will always be run but expr1 will only be run if expr1 is falsyif (expr2 || expr1) { expr3; }//Application - Writing buggy codes like this:

// exprThatShallAlwaysBeRun won't run if this._var1 is already truthythis._var1 = this._var1 || exprThatShallAlwaysBeRun;//When it should be something like this:

// Ensures exprThatShallAlwaysBeRun will indeed always be runvar1 = exprThatShallAlwaysBeRun;this._var1 = this._var1 || var1;//

Not knowing Javascript ES5 only has global and function scopes(corresponding to global and local variables)

Spoiler

Example - Treating this:

someFunction = function(target) { var index = someOtherFunction(target); for (var index = 0; index < target; index++) { yetSomeOtherFunction(index, target); } // index is now target - 1 instead of someOtherFunction(target) target = someOtherFunctionAgain(); yetSomeOtherFunctionAgain(index, target);} // someFunctionAs the same as this:

someFunction = function(target) { var index = someOtherFunction(target); for (var i = 0; index < target; index++) { yetSomeOtherFunction(i, target); } target = someOtherFunctionAgain(); yetSomeOtherFunctionAgain(index, target);} // someFunction

Not knowing return only works for the innermost function

Spoiler

Example - Treating this:

someFunction = function(array, val) var tempArray = array.slice(0); tempArray.sort(function(a, { return a.someProp - b.someProp; }); for (var index = 0, length = tempArray.length; index < length; index++) { if (tempArray[index].someProp > val) { return tempArray[index]; } } return null;} // someFunctionAs the same as this:

someFunction = function(array, val) var tempArray = array.slice(0); tempArray.sort(function(a, { return a.someProp - b.someProp; }); tempArray.forEach(function(element) { // return in forEach is the same as continue in for if (element.someProp > val) { return element; } } ); return null;} // someFunction

I'll continue to share more and more later. Let's share Javascript ES5 coding mistakes you know as well

 

[DoubleX]

Adding properties to Object.prototype without making them not enumerable

Spoiler

Spoiler





Using an old version of DoubleX RMMV Object Properties as an example:



Spoiler







/* cond: The function checking whether an object property will be traced
label: The function returning the label of an traced object property */
Object.prototype.trace_obj_prop = function(cond, label) { // New
if (!this._obj_prop_trace) {
this._obj_prop_log = {};
this._obj_prop_trace = {};
}
// Stop tracing the object property if the path would be cyclic
if (this._obj_prop_trace[cond]) {
return;
}
//
this._obj_prop_log[cond] = "";
this._obj_prop_trace[cond] = {};
this.log_obj_prop(cond, label);
}; // Object.prototype.trace_obj_prop

/* cond: The function checking whether an object property will be traced
label: The function returning the label of an traced object property */
Object.prototype.log_obj_prop = function(cond, label) { // New
// Checks if the currently traced object property has object properties
var has_obj_prop = false;
for (var prop in this) {
if (this.hasOwnProperty(prop) && this.is_obj_prop(prop)) {
has_obj_prop = true;
break;
}
}
//
if (has_obj_prop) {
this._obj_prop_log[cond] = "{";
this.traverse_obj_prop_tree(cond, label);
this._obj_prop_log[cond] += "}";
}
}; // Object.prototype.log_obj_prop

/*----------------------------------------------------------------------------
* Label and use all nonempty subtrees to form the object property tree
*----------------------------------------------------------------------------*/
/* cond: The function checking whether an object property will be traced
label: The function returning the label of an traced object property */
Object.prototype.traverse_obj_prop_tree = function(cond, label) { // New
var op = DoubleX_RMMV.Obj_Prop;
for (var prop in this) {
if (this.hasOwnProperty(prop) && this.is_obj_prop(prop)) {
var obj = this[prop];
// Recursively traverse the property tree via Depth First Search
if (op[cond](obj)) {
this._obj_prop_log[cond] += " " + prop + ": " +
op[label](obj) + " ";
this._obj_prop_trace[cond][obj] = [prop];
}
var temp_prop = prop;
if (obj !== null && typeof obj === "object") {
obj.trace_obj_prop(cond, label);
if (Object.keys(obj._obj_prop_trace[cond]).length > 0) {
if (this._obj_prop_trace[cond][obj] === undefined) {
this._obj_prop_trace[cond][obj] = [];
}
this._obj_prop_log[cond] += " " + temp_prop + ": " +
obj._obj_prop_log[cond];
this._obj_prop_trace[cond][obj].push(
obj._obj_prop_trace[cond]);
}
}
//
}
}
}; // Object.prototype.traverse_obj_prop_tree

// prop: The current object property to be traced
Object.prototype.is_obj_prop = function(prop) { // New
// Return false for all object properties added by this plugin
if (prop === "_obj_prop_log" || prop === "_obj_prop_trace") {
return false;
}
if (prop === "trace_obj_prop" || prop === "log_obj_prop") {
return false;
}
return prop !== "traverse_obj_prop_tree" && prop !== "is_obj_prop";
//
}; // Object.prototype.is_obj_prop

The above code itself seems to have no bugs in isolation, but all objects using for in loop will have troubles. It's because all the above new Object.prototype properties are enumerable.


For instance, adding the above code will cause RMMV to crash upon game start due to this:

Spoiler

Spoiler






ImageManager.isReady = function() {
for (var key in this._cache) {
var bitmap = this._cache[key];
if (bitmap.isError()) {
throw new Error('Failed to load: ' + bitmap.url);
}
if (!bitmap.isReady()) {
return false;
}
}
return true;
};

Now for (var key in this._cache) will also enumerate all the new Object.prototype properties added by the old version of DoubleX RMMV Object Properties, as they're enumerable. This results in crashes when running bitmap.isError().


To fix that, DoubleX RMMV Object Properties can be updated into this:

Spoiler

Spoiler






Object.defineProperties(Object.prototype, {

"trace_obj_prop": {
/* cond: The function checking whether an object property will be traced
* label: The function returning the label of an traced object property
*/
value: function(cond, label) { // New
if (!this._obj_prop_trace) {
this._obj_prop_log = {};
this._obj_prop_trace = {};
}
// Stop tracing the object property if the path would be cyclic
if (this._obj_prop_trace[cond]) { return; }
//
this._obj_prop_log[cond] = "";
this._obj_prop_trace[cond] = {};
this.log_obj_prop(cond, label);
}, // Object.prototype.trace_obj_prop
writable: true,
configurable: true
},

"log_obj_prop": {
/* cond: The function checking whether an object property will be traced
* label: The function returning the label of an traced object property
*/
value: function(cond, label) { // New
// Checks if currently traced object property has object properties
var has_obj_prop = false;
for (var prop in this) {
if (this.hasOwnProperty(prop) && this.is_obj_prop(prop)) {
has_obj_prop = true;
break;
}
}
//
if (!has_obj_prop) { return; }
this._obj_prop_log[cond] = "{";
this.traverse_obj_prop_tree(cond, label);
this._obj_prop_log[cond] += "}";
}, // Object.prototype.log_obj_prop
writable: true,
configurable: true
},

/*------------------------------------------------------------------------
* Label and use all nonempty subtrees to form the object property tree
*------------------------------------------------------------------------*/
"traverse_obj_prop_tree": {
/* cond: The function checking whether an object property will be traced
* label: The function returning the label of an traced object property
*/
value: function(cond, label) { // New
var op = DoubleX_RMMV.Obj_Prop;
for (var prop in this) {
if (this.hasOwnProperty(prop) && this.is_obj_prop(prop)) {
var obj = this[prop];
// Recursively traverse property tree via Depth First Search
if (op[cond](obj)) {
this._obj_prop_log[cond] += " " + prop + ": " +
op[label](obj) + " ";
this._obj_prop_trace[cond][obj] = [prop];
}
var temp_prop = prop;
if (obj === null || typeof obj !== "object") { continue; }
obj.trace_obj_prop(cond, label);
if (Object.keys(obj._obj_prop_trace[cond]).length > 0) {
if (this._obj_prop_trace[cond][obj] === undefined) {
this._obj_prop_trace[cond][obj] = [];
}
this._obj_prop_log[cond] += " " + temp_prop + ": " +
obj._obj_prop_log[cond];
this._obj_prop_trace[cond][obj].push(
obj._obj_prop_trace[cond]);
}
//
}
}
}, // Object.prototype.traverse_obj_prop_tree
writable: true,
configurable: true
},

"is_obj_prop": {
// prop: The current object property to be traced
value: function(prop) { // New
// Return false for all object properties added by this plugin
if (prop === "_obj_prop_log" || prop === "_obj_prop_trace") {
return false;
} else if (prop === "trace_obj_prop" || prop === "log_obj_prop") {
return false;
}
return prop !== "traverse_obj_prop_tree" && prop !== "is_obj_prop";
//
}, // Object.prototype.is_obj_prop
writable: true,
configurable: true
}

});

It works as properties defined via Object.defineProperties aren't enumerable by default, thus making all the new Object.prototype properties not enumerable.


On a side note: Even just adding something to Object.prototype itself is already playing with fire. It should be the last resort and done with extreme caution. The above mistake's just 1 of the many possible such dangers due to insufficient carefulness.


 

 

[Tsukihime]

If I ever need to iterate over an object's properties and I don't want to deal with the inherited stuff, I just use hasOwnProperty


Or, I just get the keys using Object.keys( SOME_OBJ)  and then iterate over that. I'm not sure which one is more efficient, and it would be nice if I could have "for ... in ..." automatically only take stuff that isn't inherited. But oh well.

 

[DoubleX]

Using objects as keys of other objects


Yesterday I realized I've made an incredibly silly yet devastating mistake that have already wasted me hours of efforts long, long ago:


DoubleX RMMV Unison Item YEP_X_BattleSysCTB -

Spoiler

/*------------------------------------------------------------------------
* Executes the async unison item when all unison actors inputted it
*------------------------------------------------------------------------*/
// item: The current async unison skill/item
GBBCTB.markAsyncUnisonItemActors = function(item) { // v1.00b+
if (!BattleManager.asyncUnisonItems[item]) {
BattleManager.asyncUnisonItems[item] = [];
}
var actors = BattleManager.asyncUnisonItems[item];
// The 1st actor in the async unison skill/item actor list's the invoker
actors.push(this);
if (actors.length !== this._unisonItemActors.length) { return; }
var act = this.currentAction(), func = 'setupUnisonCTBCharge';
BMCTB.callUnisonActors.call(BattleManager, actors[0], act, func);
//
}; // GBBCTB.markAsyncUnisonItemActors

DoubleX RMMV Unison Item YEP_X_BattleSysATB -

Spoiler

/*------------------------------------------------------------------------
* Executes the async unison item when all unison actors inputted it
*------------------------------------------------------------------------*/
// item: The current async unison skill/item
GBBATB.markAsyncUnisonItemActors = function(item) {
if (!BattleManager.asyncUnisonItems[item]) {
BattleManager.asyncUnisonItems[item] = [];
}
var actors = BattleManager.asyncUnisonItems[item];
// The 1st actor in the async unison skill/item actor list's the invoker
actors.push(this);
if (actors.length !== this._unisonItemActors.length) { return; }
var actorIds = this._unisonItemActors, func = 'setupATBCharge';
BMATB.callUnisonActors.call(BattleManager, actors[0], actorIds, func);
actors.forEach(function(actor) {
if (actor) { actor.isUnisonItemReady = true; }
});
//
}; // GBBATB.markAsyncUnisonItemActors

The mistake is that, I've used item, which is an object, as a key of BattleManager.asyncUnisonItems, which is another object.


While I know that object keys will be stringified, I still thought that those stringified keys will still be unique, as each item object has an unique id making them unique.


So, for example, let's say the id of item1 is 1 and that of item10 is 10, the below's what I expect to happen:


BattleManager.asyncUnisonItems = {};
BattleManager.asyncUnisonItems[item1] = [actor1];
alert(BattleManager.asyncUnisonItems[item10].toSource()); // Expected to crash due to calling toSource() for null



What actually happens is this:


BattleManager.asyncUnisonItems = {};
BattleManager.asyncUnisonItems[item1] = [actor1];
alert(BattleManager.asyncUnisonItems[item10].toSource()); // Showing [actor1] in the message box



So using objects as keys of other objects can actually end up losing the uniqueness of those keys even when those objects are themselves unique.


In DoubleX RMMV Unison Item YEP_X_BattleSysCTB, while this mistake sometimes led to awkward and inconsistent behaviors, the overall mechanisms still seem to be working, so I thought using objects as keys of other objects were just code smells or perhaps an anti-pattern(but convenient one).


In DoubleX RMMV Unison Item YEP_X_BattleSysATB, however, the plugin just didn't work at all before I realized this mistake. On the bright side, I've learned a hard lesson in Javascript ES5 due to hours of debugging lol


I've searched on the internet and that's why:


https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys





// array like object with random key ordering
var an_obj = { 100: 'a', 2: 'b', 7: 'c' };
console.log(Object.keys(an_obj)); // console: ['2', '7', '100']




https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/toString





var o = new Object();
o.toString(); // returns [object Object]






In my case, when using any item object as a key of other objects, that item object will be stringified to "[object Object]" via item.toString().


Let's check the aforementioned example code again:


BattleManager.asyncUnisonItems = {};
BattleManager.asyncUnisonItems[item1] = [actor1];
alert(BattleManager.asyncUnisonItems[item10].toSource());



I thought what's happening under the hood is this:


BattleManager.asyncUnisonItems = {};
BattleManager.asyncUnisonItems["item1UniqueContents"] = [actor1];
alert(BattleManager.asyncUnisonItems["item10UniqueContents"].toSource());



But what's really happening is this:


BattleManager.asyncUnisonItems = {};
BattleManager.asyncUnisonItems["[object Object]"] = [actor1];
alert(BattleManager.asyncUnisonItems["[object Object]"].toSource());




That's why alert(BattleManager.asyncUnisonItems[item10].toSource()); will show [actor1] in the message box instead of crashing the game

Therefore, using objects as keys of other objects this way will always lead to the loss of uniqueness of those keys even when all those objects are themselves unique.


In short: Using objects as keys of other objects in Javascript ES5 isn't a code smell nor anti-pattern, but just outright a mistake.