Skip to content
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

fix(runtime-core/scheduler): use undefined instead of null #1816

Closed
wants to merge 1 commit into from
Closed

fix(runtime-core/scheduler): use undefined instead of null #1816

wants to merge 1 commit into from

Conversation

Picknight
Copy link
Contributor

@Picknight Picknight commented Aug 9, 2020

  1. use undefined instead of null in getId function, because the value of the undeclared job.id is undefined
  2. return early in flushPreFlushCbs and flushPostFlushCbs function for better readability
// before
export function flushPostFlushCbs() {
  if (pendingPostFlushCbs.length) {
    // do more
  }
}

// after
export function flushPostFlushCbs() {
  if (!pendingPostFlushCbs.length) return
  // do more
}

@dsonet
Copy link
Contributor

dsonet commented Aug 13, 2020

Talking about the readability, I think the current one is better. It's clearer to have the logic in an if branch, focus on what we should deal with. With an extra return, it does reduce the indent but definitely increase the cost of reading the code, you have to keep reminding yourself it's actually introduced an else branch.

@Picknight
Copy link
Contributor Author

Talking about the readability, I think the current one is better. It's clearer to have the logic in an if branch, focus on what we should deal with. With an extra return, it does reduce the indent but definitely increase the cost of reading the code, you have to keep reminding yourself it's actually introduced an else branch.

Readability is actually a subjective concept, I just think that most people’s default specifications should be followed. The Return Early Pattern :)

@yyx990803
Copy link
Member

== null checks for both null and undefined. In this case it doesn't really make a difference. As for the early return - we generally don't encourage readability related PRs since they are subjective but usually ends up in messy diffs that makes the git history harder to follow.

@yyx990803 yyx990803 closed this Aug 14, 2020
@Picknight
Copy link
Contributor Author

Picknight commented Aug 15, 2020

Thanks for your explanation @yyx990803 , I got it.

@Picknight Picknight deleted the picknight/scheduler branch August 18, 2020 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants