Repository navigation
Object setter orders sets based on size of unit - #3008
maggiepint wants to merge 1 commit into
Conversation
| for (unit in units) { | ||
| this.set(unit, units[unit]); | ||
| units = normalizeObjectUnits(units); | ||
| // for (unit in units) { |
|
My only real feedback is that it might be easier to manage the priorities by having them all in the same place, like in an object in get-set.js. The tradeoff is that if we add a unit it would be easy to forget. The upside is that it's easy to see the expected behavior of get-set just looking at the one file. |
| // this.set(unit, units[unit]); | ||
| // } | ||
| var prioritized = getPrioritizedUnits(); | ||
| for (var i = 0; i < prioritized.length; i++) |
There was a problem hiding this comment.
What I'd suggest instead, is make a function that takes units and a callback and calls it in the right order. Normally you'd set just a few units, and that will just sort those units and callback with them. In the future if we add 15 calendar systems we'll get hundreds of units, and we have to iterate them every time -- it should be O(number-of-units-to-be-set) not O(number-of-total-units).
There was a problem hiding this comment.
I'm not sure you need a callback, but I like the idea of sorting the units provided instead of the units known. For each unit, look it up in the map that gives you the priority; that gives you a list of unit/priority pairs. Sort the list by priority. IOW, getPrioritizedUnit(units) returns just those units in the right order.
There was a problem hiding this comment.
The identity lookup makes sense. When I wrote this code I was thinking 'how many unit types could possibly be added', but I suppose if calendar systems were added, a lot.
2446619 to
4ddc0bb
Compare
|
So, I got this down to O(2*numberOfUnitsToBeSet) plus a sort call that is probably n log n. |
|
I think you can actually make it just a function of the number of units (technically O(1), though not, obviously, necessarily faster). Here's what I'd do (untested, probably broken): var prioritized = ['year', 'month', //etc
function getSet(obj){
if (typeof units === 'object') {
units = normalizeObjectUnits(units);
for (var unit of prioritized) //don't know if we're doing for..of yet, but it sure is nice
this[unit](units[unit]);Obvious downside: have to iterate through units you don't actually have in the object. Upsides: single pass, no sorting, priorities defined in one place (or, if you want to distribute the priority definitions, sorted once and cached). |
|
That's basically the same as what I had before we decided we only wanted to iterate over used units :-) |
|
Haha, I just read back and saw that. Consistency is apparently not my strong suit. |
|
After thinking about this more, I'm +1 on the code as it is now. |
|
I'm marking this pending release since @icambron is okay with it. |
|
Merged in 00d924a |
Object setter orders sets based on size of unit
Current code for using an object to set multiple values uses for ... in.
MDN is quite clear on the fact that this runs in arbitrary order:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in
Order matters when values are being set.
The object setter should set the values in order of unit size (Years being biggest, milliseconds being smallest). This prevents errors like:
It is a minor separate change, but this also fixes an error where calling .get with an object will set the values passed in the object: