The Wayback Machine - http://web.archive.org/web/20201031145523/https://github.com/ryanmcdermott/clean-code-javascript/pull/320
Skip to content

/ clean-code-javascript

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

Jump to bottom

Add calculation for milliseconds in a day #320

Merged
merged 1 commit into from Oct 7, 2020
Merged

Add calculation for milliseconds in a day #320

merged 1 commit into from Oct 7, 2020

Conversation

@ychek
Copy link
Contributor

@ychek ychek commented Sep 6, 2020

Hey,

I think it's a bit more clear like that. WDYT?
Also there was a little typo because 86_400_000 is not a number.
@ychek
Add calculation for milliseconds in a day
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
365717f
@zachlysobey
Copy link

@zachlysobey zachlysobey commented Sep 6, 2020

I like it a little better like this too.
A quick note though: that number is fine. Or at least, valid as a stage-4 TC39 proposal and available pretty widely.

https://tc39.es/proposal-numeric-separator/
@ychek
Copy link
Contributor Author

@ychek ychek commented Sep 7, 2020

You are right @zachlysobey!
That number is fine, and I was not aware of that numeric separator that are an amazing feature BTW. ;)
However we agree that calculation is important to better understand that number.
@ryanmcdermott
Copy link
Owner

@ryanmcdermott ryanmcdermott commented Oct 7, 2020

Thanks! The only advantage to storing it as a flat constant is that you don't need to worry about re-running computations unnecessarily but as far as I understand V8 (as of this year) performs constant folding, and likely you could get the same benefit in TypeScript compiler optimization if they don't exist already.
@ryanmcdermott ryanmcdermott merged commit 8f678c8 into ryanmcdermott:master Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@ychek @zachlysobey @ryanmcdermott
You can’t perform that action at this time.