Please Stop Reassigning For No Reason!
I swear it is was a short one but a must write and yes, once again, since I keep seeing this kind of mistake everywhere!
This is not about JavaScript, this is about programming whatever language you want ... if you do this, you are doing it wrong!
Performances a part, this pattern can be also dangerous, specially in this ES5 era where getters and setters are extremely common, specially on mobile browsers where nobody cares about IE gap.
I am looking at you only if you are still writing something like this in your code:
where window is just the generic object example.
This simply means that:
No getters or setters? It does not matter since in this way any property has to be retrieved and, if present, has to be reassigned to itself.
If you don't believe me you can simply check the Webkit engine Object source code and compare operations needed to set and get VS JSObjectHasProperty, basically just a call to jsObject->hasProperty rather than a whole logic moved via JSObjectGetProperty plus JSObjectSetProperty, and of course, invoking hasProperty as well.
The reason it's better is simple: no setters or getters invoked plus no redundant+superfluous operations performed over *reassignment*. Above snippet will simply invoke JSObjectHasProperty and jsObject->hasProperty so that the whole setter logic will be executed only if necessary and in any case no getter logic will ever be involved ... got it?
Since defined properties respect the in operator, nobody blocks us to define them using same pattern.
and modify accordingly. The replace RegExp requires unfortunately the end of the assignment so it may be weak.
However, above technique still invokes the getter, either defined by the user or behind the JavaScript scene ( in core ).
My first suggestion avoids "empty getters" so even if this latter technique may be considered a better approach, it can still suffers or imply side effects.
Update On False Positives
As Diego pointed out the "prop" in obj may result into a false positive if obj.prop is assigned but it is falsy. I consider this really an edge case and even such pattern will be error prone if the value is truish but not the expected one.
There is actually no easy or fast enough way to compare two clones runtime in order to understand if the assigned object is the one expected. Also at that point this extra overhead would be unnecessary since re-assignment will be just faster.
At the end of the day it's up to us to be as safe as possible still preserving common sense and good programming logic but don't tell me I am talking about premature optimizations because all I am talking about is logic over a pattern that is similar in size with the one I am suggesting but it's completely different in therms of required operations and destroyed possibilities.
We should never abuse bad practices behind the "premature" flag!
As asked via comments, I have created a jsperf benchmark. Please bear in mind the used test is not a real use case while described problems are still the same I have been talking about in this post.
JIT compilers may optimize repeated code and this is most likely what happens with the commonPattern approach.
I love the fact the benchmark basically proves me wrong, when getters and setters are not involved on JS side, because it's kinda illogical accordingly with browsers implementations.
Said that, if anybody from Chrome development would be so kind to comment why re-assignment is faster than just in it would be really nice.
As example, there is only one proper way to shim Array.isArray.
If we use return typeof obj.length == "number" rather than toString.call(obj) == "[object Array]"we will surely go faster but we will do it totally wrong as well.
Last note about Webkit, we have to deal with it since it has the majority of mobile browsing market share. Special case is webOS which implements V8 rather than JSC but still, don't be blind in front of millions, just understand side effects the common pattern could cause against the right way to know if a property has been set already.
This is not about JavaScript, this is about programming whatever language you want ... if you do this, you are doing it wrong!
The Problem
JS Engines developers are desperate! They are even posting how to help JIT compilers to go faster and developers seem to be so lazy that even most basic good practices are avoided.Performances a part, this pattern can be also dangerous, specially in this ES5 era where getters and setters are extremely common, specially on mobile browsers where nobody cares about IE gap.
I am looking at you only if you are still writing something like this in your code:
window.whatever = window.whatever || {
/* the whatever it is, object, function, anything */
};
where window is just the generic object example.
OMG, What Can Be Wrong ...
Everything! Any object property could be a native or user defined setter. In this case above technique is invoking the potential setter passing through the potential getter.This simply means that:
- accordingly to the task, we are asking N extra computations or function invokes for no reason
- the setter may accept something different from what the getter returns. If this is true, result could be an error, rather than a "smart assignment"
- if the setter was set already and it was a lazy re-assignment logic, we are avoiding lazy assignment features/logic requiring it's execution instantly and, once again, without any reason
- if the object has getter only the operation will throw an error while setting
No getters or setters? It does not matter since in this way any property has to be retrieved and, if present, has to be reassigned to itself.
How JavaScript Engines Work
Thanks for asking. Let's imagine every object refers behind the scene to a stack of strings representing the list of properties. This stacks is used to know if object.hasOwnProperty("propertyName") or if "propertyName" in object is present. Once decided if the object can access the propertyName, a sort of -1 < objectProperties.indexOf("propertyName") procedure, the property has to be retrieved and eventually unboxed. Once this part is complete, the reassignment does not care much about the same property. Here comes the propertyName to propertyValue procedure which most likely will "erase" the older reference to reassign the new one. Even if the engine is truly smart, all possible checks/logics about memory address for the value and property name for the object has to be executed.If you don't believe me you can simply check the Webkit engine Object source code and compare operations needed to set and get VS JSObjectHasProperty, basically just a call to jsObject->hasProperty rather than a whole logic moved via JSObjectGetProperty plus JSObjectSetProperty, and of course, invoking hasProperty as well.
How To Do The Same Better And Faster
"whatever" in window || (window.whatever = {
/* the whatever it is, object, function, anything */
});
// eventually easier to shortcut via minifier
var key = "whatever";
key in window || (window[key] = {
/* the whatever it is, object, function, anything */
});
// eventually easier to use but requires potentially extra memory
// 'cause the assignment has to be created in any case
// and it can't be ignored as it could be with precedent examples
function setDefault(object, key, value) {
key in object || (object[key] = value);
// here we can play with the returned value
// I chose the function itself but it can be anything
// or nothing if you prefer
return setDefault;
}
setDefault(
window, "whatever", {}
)(
window, "somethingElse", function(){}
)(
window, "howCoolIsIt", "very cool!"
);
The reason it's better is simple: no setters or getters invoked plus no redundant+superfluous operations performed over *reassignment*. Above snippet will simply invoke JSObjectHasProperty and jsObject->hasProperty so that the whole setter logic will be executed only if necessary and in any case no getter logic will ever be involved ... got it?
It Does Not Work
Oh ... Really? Most likely it's a browser bug you should file ASAP and even most likely a new feature not there yet. The in operator should always work as expected indeed with or without inheritance involved.
var o = Object.defineProperty({}, "test", {
enumerable: false,
writable: false,
configurable: false,
value: "OK"
});
alert("test" in o && o.test); // ... guess what ...
// OK
Since defined properties respect the in operator, nobody blocks us to define them using same pattern.
"prop" in object || Object.defineProperty(
object, "prop", descriptor
);
I Have To Do Refactoring
Good, so start with this RegExp to search the evil code in all your failes:
/([$_0-9a-zA-Z]+(?:\.[$_0-9a-zA-Z]+|\[[$_0-9a-zA-Z]+\]))\s*=\s*\1/
.test(fileText)
// or suggested by @joseanpg
/([$\w]+(?:\.[$\w]+|\[[$\w]+\]))\s*=\s*\1(?:[\s\|&,;]|$)/m
and modify accordingly. The replace RegExp requires unfortunately the end of the assignment so it may be weak.
I Kill A Kitten Each Time I Read That
This is what happens if you don't start now with the right way so, please, THINK ABOUT KITTENS!Update On Alternative Ways
As @diegoperini pointed out in this tweet another way to avoid the setter is:
window.propertyName || (window.propertyName = {
/* the whatever value */
});
However, above technique still invokes the getter, either defined by the user or behind the JavaScript scene ( in core ).
My first suggestion avoids "empty getters" so even if this latter technique may be considered a better approach, it can still suffers or imply side effects.
Update On False Positives
As Diego pointed out the "prop" in obj may result into a false positive if obj.prop is assigned but it is falsy. I consider this really an edge case and even such pattern will be error prone if the value is truish but not the expected one.
this.prop || (this.prop = {});
// now imagine before
this.prop = true;
// bye bye library
There is actually no easy or fast enough way to compare two clones runtime in order to understand if the assigned object is the one expected. Also at that point this extra overhead would be unnecessary since re-assignment will be just faster.
At the end of the day it's up to us to be as safe as possible still preserving common sense and good programming logic but don't tell me I am talking about premature optimizations because all I am talking about is logic over a pattern that is similar in size with the one I am suggesting but it's completely different in therms of required operations and destroyed possibilities.
We should never abuse bad practices behind the "premature" flag!
Update With Benchmark
As asked via comments, I have created a jsperf benchmark. Please bear in mind the used test is not a real use case while described problems are still the same I have been talking about in this post.
JIT compilers may optimize repeated code and this is most likely what happens with the commonPattern approach.
I love the fact the benchmark basically proves me wrong, when getters and setters are not involved on JS side, because it's kinda illogical accordingly with browsers implementations.
Said that, if anybody from Chrome development would be so kind to comment why re-assignment is faster than just in it would be really nice.
Conclusions
100000 VS 2000000 ops for non real cases are not so useful to analyze but what should be underlined is that specially with getters and setter the in operator is both faster and safer cross browser and cross platform.As example, there is only one proper way to shim Array.isArray.
If we use return typeof obj.length == "number" rather than toString.call(obj) == "[object Array]"we will surely go faster but we will do it totally wrong as well.
Last note about Webkit, we have to deal with it since it has the majority of mobile browsing market share. Special case is webOS which implements V8 rather than JSC but still, don't be blind in front of millions, just understand side effects the common pattern could cause against the right way to know if a property has been set already.
Comments
Post a Comment