Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

Code optimization for datepicker.js #5941

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Code optimization for datepicker.js #5941

wants to merge 2 commits into from

Conversation

saket-joshi
Copy link

No description provided.

@wesleycho
Copy link
Contributor

This should not have a merge commit in history.

@wesleycho
Copy link
Contributor

Is this an optimization? I believe that the change should be slower than the original.

@saket-joshi
Copy link
Author

  • Actually not! Since $scope.datepickerOptions = $scope.datepickerOptions || {} is expression rather than a condition.
  • In case $scope.datepickerOptions does not have a value, a blank object would be returned in the same statement. If we use the if.. condition block, the "!" condition would still be checked even if it has a value.

@pkozlowski-opensource
Copy link
Member

Guys, this is probably a case of a micro-optimization that can trigger a nice discussion but probably will have negligible impact in practice - unless has numbers that prove things otherwise! So, let's have something that is more readable.

@icfantv
Copy link
Contributor

icfantv commented Jun 13, 2016

@saket-joshi, if you would like this to remain open, please produce numbers with associated code that indicates this is a performance improvement. Otherwise, this will be closed. Thanks.

@ghost
Copy link

ghost commented Dec 27, 2016

Actually this "micro-optimization" is slower than a condition.
https://jsbench.github.io/#7bdb181e8c7e4fd249726e4b8159d63d When values exists.
https://jsbench.github.io/#0e971943cf0e394f910cfdf1f6918abe When value does not exist.

In Firefox 50.0 $scope.object = $scope.object || {}; is 3 times slower than if (!$scope.object) {$scope.object = {};}
Google Chrome performs better but still the if case is faster.

Maybe my test Setup and Teardown are not correct, but I doubt it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants