[misc] Use switch-case for better get-set performance (#2659) #5718
Conversation
Also done for `setMonth`. Also simplified Leap Day logic.
| d = mom._d; | ||
| isUTC = mom._isUTC; | ||
|
|
||
| switch (unit) { |
There was a problem hiding this comment.
How did you verify all units that are used in the getter and setter?
There was a problem hiding this comment.
This function get(mom, unit) is actually only used internally.
The public method mom.get(unit) is actually the function stringGet(unit) which checks for this.
This is exactly what protected the old code from throwing.
I'm just returning NaN instead, the same as for invalid mom.
Indeed a behavior change I allowed myself, but only for a private function, which doesn't seem like it's designed to ever throw. I also believe any dev will get and early enough error from the NaNa anyway if that's a concern.
Did I get it right?
Should I still throw something?
src/lib/moment/get-set.js
Outdated
| return isUTC ? d.getUTCHours() : d.getHours(); | ||
| case 'Date': | ||
| return isUTC ? d.getUTCDate() : d.getDate(); | ||
| // case 'Day': return isUTC ? d.getUTCDay() : d.getDay(); // Not used |
There was a problem hiding this comment.
Sorry for leaving the comments in. There no problem or anything. It's just that that getSetDayOfWeek has it's own implementation instead of makeGetSet('Day', true) and it doesn't call get(this, 'Days') like getSetMonth does.
This is actually the reason get('day') performance was higher than the rest :)
Anyway, now that wouldn't matter with new code, so I'll make sure get(this, '...') is used where is can be used as part of this commit. That looks like 1 LOC in a couple of files.
There was a problem hiding this comment.
Done!
Now using get in getSetDayOfWeek.
I ended up leaving the comments in set() and added the reason we cannot do this for setMonth.
It can be used there too, if I implement separate private functions instead of get and set.
I'd love to do that too, but that's a bigger move and I think should be discussed separately.
- Use it in `getSetDayOfWeek` - Also update style and comments for commented out `set()` cases
|
@marwahaha, @ichernev, BTW, this is just the first step (step zero were the extra benchmarks), and I have more impactful improvements waiting. I'd love to see all these improvements go through, following that to define best practices for maintaining them. |
|
Merged in e64ced7 |
[misc] Use switch-case for better get-set performance (#2659)
Also done for
setMonth.Also simplified Leap Day logic.