New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object setter orders sets based on size of unit #3008

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
4 participants
@maggiepint
Member

maggiepint commented Mar 7, 2016

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:

moment('2016-04-30').set({date:31, month:4}).format()
"2016-05-01T00:00:00-05:00"

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:

moment('2016-04-30').get({year:2017}).format()
"2017-04-30T00:00:00-05:00"

@mj1856 mj1856 added the Bug-fix label Mar 8, 2016

for (unit in units) {
this.set(unit, units[unit]);
units = normalizeObjectUnits(units);
// for (unit in units) {

This comment has been minimized.

@icambron

icambron Apr 10, 2016

Member

kill the commented-code

@icambron

This comment has been minimized.

Member

icambron commented Apr 10, 2016

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++)

This comment has been minimized.

@ichernev

ichernev Apr 11, 2016

Contributor

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).

This comment has been minimized.

@icambron

icambron Apr 11, 2016

Member

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.

This comment has been minimized.

@maggiepint

maggiepint Apr 11, 2016

Member

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.

@maggiepint

This comment has been minimized.

Member

maggiepint commented Apr 24, 2016

So, I got this down to O(2*numberOfUnitsToBeSet) plus a sort call that is probably n log n.
If there is a way to get this down to O(numberOfUnitsToBeSet) I am too dumb to know it.
I don't see a way around having to retrieve and sort the units first, and then iterate over them again to set them.
If we had support for .filter I could populate a sparse array and then squash the unneeded values out and that could maybe squeeze some more out of it as compared to the sort call.
On the up side, I thought hard about the performance of a sort for the first time since college, so that was fun.

@icambron

This comment has been minimized.

Member

icambron commented Apr 25, 2016

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).

@maggiepint

This comment has been minimized.

Member

maggiepint commented Apr 25, 2016

That's basically the same as what I had before we decided we only wanted to iterate over used units :-)

@icambron

This comment has been minimized.

Member

icambron commented Apr 25, 2016

Haha, I just read back and saw that. Consistency is apparently not my strong suit.

@icambron

This comment has been minimized.

Member

icambron commented Apr 25, 2016

After thinking about this more, I'm +1 on the code as it is now.

@maggiepint

This comment has been minimized.

Member

maggiepint commented May 1, 2016

I'm marking this pending release since @icambron is okay with it.

@mj1856 mj1856 added this to the 2.14.0 milestone May 10, 2016

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jun 14, 2016

Merged in 00d924a

@ichernev ichernev closed this Jun 14, 2016

ichernev added a commit that referenced this pull request Jun 14, 2016

Merge pull request #3008 from maggiepint:objectSetter
Object setter orders sets based on size of unit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment