Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Simplify watchGroupFn assignment using ternary #7619

Closed
wants to merge 1 commit into from

Conversation

toddmotto
Copy link

Request Type: refactor

How to reproduce:

Component(s): scope

Impact:

Complexity: small

This issue is related to: performance regression

Detailed Description:

Other Comments:

Assigning true/false I've changed to use a ternary operator as it's better suited.

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7619)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

} else {
watchGroupFn.$$unwatch = false;
}
watchGroupFn.$$unwatch = unwatchCount === 0 ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could go even further and say watchGroupFn.$$unwatch = !!!unwatchCount;

But I'm not sure if this kind of thing is going to get merged, because the style guide is pretty clear about preferring single expressions per line, and not necessarily approving of ternary expressions.

I'll see if we can do this though

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did think about something similar... such as !(!!unwatchCount); to make things a little less crazy ;) but yes... okay and thanks :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toddmotto I think #7486 (comment) is probably showing that this isn't going to land, even if it's concise.

Minifiers could possibly do something cute like this, though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure which comment, and okay! :)

Sent from my iPhone

On 28 May 2014, at 22:19, Caitlin Potter notifications@github.com wrote:

In src/ng/rootScope.js:

@@ -400,11 +400,7 @@ function $RootScopeProvider(){

     deregisterFns.push(self.$watch(watchGroupFn, function () {
       listener(newValues, oldValues, self);

Minifiers could possibly do something cute like this, though.


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I specifically linked to one of Igor's review comments from the PR which implemented this --- @lgalfaso originally used a similar expression, and was asked to replace it with if / else blocks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh okay. I don't think the GitHub mobile on iOS takes me to the comment in that case. Will check out on desktop!

Sent from my iPhone

On 28 May 2014, at 22:40, Caitlin Potter notifications@github.com wrote:

In src/ng/rootScope.js:

@@ -400,11 +400,7 @@ function $RootScopeProvider(){

     deregisterFns.push(self.$watch(watchGroupFn, function () {
       listener(newValues, oldValues, self);
  •      if (unwatchCount === 0) {
    
  •        watchGroupFn.$$unwatch = true;
    
  •      } else {
    
  •        watchGroupFn.$$unwatch = false;
    
  •      }
    
  •      watchGroupFn.$$unwatch = unwatchCount === 0 ? true : false;
    
    I specifically linked to one of Igor's review comments from the PR which implemented this --- @lgalfaso originally used a similar expression, and was asked to replace it with if / else blocks.


Reply to this email directly or view it on GitHub.

@btford btford added this to the Purgatory milestone May 28, 2014
@toddmotto toddmotto closed this May 29, 2014
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