Simplified units #714

Merged
merged 6 commits into from Apr 8, 2013

2 participants

@icambron
Moment.js member

Fix for #710.

@timrwood
Moment.js member

Looking good!

We can also use this here and possibly here with a for in loop or something.

@icambron
Moment.js member

Ah, I just missed the diff one. For the duration constructor, I wasn't sure if it was a good idea at the time, but now that I'm thinking about it, it'll be pretty clean with a couple of loops. I'll give it a shot and update the PR.

@icambron
Moment.js member

Woops, I was right the first time. It's really ugly to use it for the duration thing. First, we have to inverse the object:

var inverseUnitAliases = {};
(function () {
var singular, abbr;
    for (abbr in unitAliases) {
        if (unitAliases.hasOwnProperty(abbr)) {
            singular = unitAliases[abbr];
            inverseUnitAliases[singular] = [singular, singular + "s", abbr];
        }
    }
})();

That gives us {years: ["year", "years", "y"], ...}. Then we have to be able to find one of them in passed-in duration object:

    function findUnits(object, units) {
        var i, alias, aliasList;
        units = normalizeUnits(units);
        aliasList = inverseUnitAliases[units];
        if (aliasList) {
            for (i in aliasList) {
                alias = aliasList[i];
                if (object.hasOwnProperty(alias)) {
                    return object[alias];
                }
            }
        }
        return 0;
    }

Now we can say,

years = findUnits(duration, 'year'),
months = findUnit(duration, 'month'),
...

Or even

var n = {};
for (i in inverseUnitAliases) {
    if (inverseUnitAliases.hasOwnProperty(i)) {
        n[i] = findUnit(i);
    }
}

and then just use n.year everywhere instead of just year. Anyway, now I'm pretty sure we're worse-off than before. Unless you plan on adding a whole buttload more unit aliases, I'm thinking I should just do the diff() part and call it day.

@timrwood
Moment.js member

Yeah, I thought there have to be some trickery for normalizing object properties, but I didn't think it'd be this complicated. I think we can pull this in as is.

Thanks for working on this!

@timrwood timrwood merged commit 5b747a7 into moment:develop Apr 8, 2013

1 check passed

Details default The Travis build passed
@icambron icambron deleted the icambron:simplified-units branch Apr 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment