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.

Sign up for GitHub

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

crypto: fix duplicated switch-case return values #49030

Merged
merged 1 commit into from
May 12, 2024
Merged

crypto: fix duplicated switch-case return values #49030

merged 1 commit into from
May 12, 2024

Conversation

0o001
Copy link
Contributor

@0o001 0o001 commented Aug 5, 2023 •
edited

I defined the same returning values within a switch-case under a single case.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Aug 5, 2023
@tniessen
Copy link
Member

tniessen commented Aug 5, 2023

While it technically reduces the number of statements, I don't really see how this improves the code or its readability.

@0o001
Copy link
Contributor Author

0o001 commented Aug 6, 2023

While it technically reduces the number of statements, I don't really see how this improves the code or its readability.

I see the concern, but grouping returning values in a single case reduces redundancy and improves code efficiency, making it easier to maintain and read.

Even though it's a minor repetition, this constitutes a code duplication and violates the DRY (Don't Repeat Yourself) principle. As a result, you can already observe instances of the same operation being performed in various parts of the project. Despite the small magnitude of the issue, I still strive to contribute to the project in any way possible.

https://github.com/nodejs/node/blob/main/lib/internal/crypto/cfrg.js#L49
https://github.com/nodejs/node/blob/main/lib/internal/crypto/cfrg.js#L110
https://github.com/nodejs/node/blob/main/lib/internal/crypto/cfrg.js#L155
https://github.com/nodejs/node/blob/main/lib/internal/crypto/cfrg.js#L249
https://github.com/nodejs/node/blob/main/lib/internal/crypto/ec.js#L258
https://github.com/nodejs/node/blob/main/lib/internal/crypto/mac.js#L73

And more...

@panva
Copy link
Member

panva commented Aug 6, 2023

If this is going to fallthrough would you mind moving the returns on newlines to actually help readability?

0o001 added a commit to 0o001/node that referenced this pull request Aug 6, 2023
@0o001
crypto: moving the returns on newlines
5de60e5 
nodejs#49030 (comment)
@0o001
Copy link
Contributor Author

0o001 commented Aug 6, 2023

If this is going to fallthrough would you mind moving the returns on newlines to actually help readability?

Thank you for your response, I have rearranged the code as you suggested.

panva
panva approved these changes Aug 6, 2023
View reviewed changes
Copy link
Member

@panva panva left a comment

Choose a reason for hiding this comment

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

I don't think this improves much but it also doesn't stand to hurt anything.

@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 6, 2023
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2023

Failed to start CI
- Validating Jenkins credentials
✘  Jenkins credentials invalid
https://github.com/nodejs/node/actions/runs/5775909236

@panva
Copy link
Member

panva commented Aug 6, 2023

Need to wait until https://nodejs.org/en/blog/vulnerability/august-2023-security-releases is through to start CI and let this progress.

@lpinca
Copy link
Member

lpinca commented Aug 7, 2023

It is a cosmetic change. I'm approving only because there is another approval. With two approvals we can land it faster without waiting one week.

@tniessen
Copy link
Member

tniessen commented Aug 7, 2023

I see the concern, but grouping returning values in a single case reduces redundancy and improves code efficiency, making it easier to maintain and read.

I am not sure if it really improves code efficiency, and as @panva said, even if it does, efficiency really doesn't matter much here.

I don't think this is an improvement because intuitively this is a one-to-one mapping from algorithms to block sizes. One line per algorithm, the name on the left and the corresponding value on the right. Easily readable and mathematically concise.

Grouping algorithms like this makes no sense. The fact that SHA-1 and SHA-256 have the same internal block size is purely coincidental. The algorithms are vastly different.

If we keep adding algorithms to the list, this change will effectively demand ordering algorithms by their block size. For example, the SHA-3 bitrates (i.e., block sizes) fall both below and above that of SHA-512, so I guess we'd have to mix SHA-2 and SHA-3 in this list?

Anyway, I won't object, but I don't think these changes help.

@panva
Copy link
Member

panva commented Aug 8, 2023

Anyway, I won't object, but I don't think these changes help.

Same here.

@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Aug 8, 2023
@tniessen tniessen added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53108/

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53340/

@github-actions github-actions bot mentioned this pull request Aug 16, 2023
Open
24 tasks
@github-actions github-actions bot mentioned this pull request Aug 17, 2023
Open
26 tasks
@tniessen
Copy link
Member

The commit message neither matches the PR title nor does it follow the commit message guidelines. (And it seems that nobody has actually approved this PR because they think it's a good change.)

@0o001 0o001 changed the title crypto: merge similar switch-case returned column crypto: moving the returns on newlines Aug 17, 2023
This was referenced Aug 18, 2023
Open
Open
Open
@lpinca lpinca added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 26, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53565/

@tniessen
Copy link
Member

FWIW, the commit message still doesn't adhere to the commit message guidelines since it doesn't begin with an imperative verb after the prefix.

@github-actions github-actions bot mentioned this pull request Aug 27, 2023
Open
25 tasks
@0o001 0o001 changed the title crypto: moving the returns on newlines crypto: fix duplicated switch-case return values Aug 27, 2023
@github-actions github-actions bot mentioned this pull request Aug 28, 2023
Open
25 tasks
@panva panva removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 28, 2023
This was referenced Aug 29, 2023
Open
Open
Open
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59106/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59148/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59177/

@aduh95 aduh95 merged commit 2b57e47 into nodejs:main May 12, 2024
53 checks passed
@aduh95
Copy link
Contributor

aduh95 commented May 12, 2024

Landed in 2b57e47

targos pushed a commit that referenced this pull request May 12, 2024
@0o001 @targos
crypto: fix duplicated switch-case return values
0d040a3 
PR-URL: #49030
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos mentioned this pull request May 13, 2024
Merged
lukins-cz pushed a commit to lukins-cz/OS-Aplet-node that referenced this pull request Jun 1, 2024
@0o001 @lukins-cz
crypto: fix duplicated switch-case return values
a798bcc 
PR-URL: nodejs#49030
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panva panva panva approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@0o001 @nodejs-github-bot @tniessen @panva @lpinca @aduh95