crypto: Remove useless if statement and convert if-else-if chain - #14911
crypto: Remove useless if statement and convert if-else-if chain#14911starkwang wants to merge 1 commit into
Conversation
Sorry, something went wrong.
The if statement in `ECDH.getPublicKey` is useless. This change is to remove it and convert the `if-else-if` chain to a `switch` statement, which is more efficient.
bnoordhuis
left a comment
There was a problem hiding this comment.
I don't think this change is an improvement in legibility over the existing code.
Sorry, something went wrong.
| f = constants.POINT_CONVERSION_UNCOMPRESSED; | ||
| break; | ||
| default: | ||
| throw new TypeError('Bad format: ' + format); |
There was a problem hiding this comment.
This looks like it's breaking the typeof format === 'number case.
Sorry, something went wrong.
There was a problem hiding this comment.
The typeof format === 'number is no use right now. It only assign a value to the f and then throw an error without using f.
Sorry, something went wrong.
There was a problem hiding this comment.
This is true but the error should also be thrown in case the format.length will match one of the entries without matching the correct word. That is not the case anymore.
Sorry, something went wrong.
| throw new TypeError('Bad format: ' + format); | ||
| switch (format.length) { | ||
| case 6: | ||
| if (format === 'hybrid') |
There was a problem hiding this comment.
Are you sure this is faster? There are 2 checks now, the original only had one.
Sorry, something went wrong.
There was a problem hiding this comment.
It will go through all the cases in the if-else-if chain. The switch statement reduces it into 2 checks. Same code in https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L513
Sorry, something went wrong.
benjamingr
left a comment
There was a problem hiding this comment.
Sorry but I think that this hurts readability. I'd also like to see a benchmark back up any performance claims. Thanks for your contribution.
Sorry, something went wrong.
BridgeAR
left a comment
There was a problem hiding this comment.
Either each if should get a else path with the error or another if statement should be added to check for f !== undefined. The latter would remove the benefit of having two checks in average instead of 1-3. As the performance is negligible for one - three checks I think it is nicer to read with the if else as it was before. So only the obsolete if should be removed out of my perspective.
Sorry, something went wrong.
| f = constants.POINT_CONVERSION_UNCOMPRESSED; | ||
| break; | ||
| default: | ||
| throw new TypeError('Bad format: ' + format); |
There was a problem hiding this comment.
This is true but the error should also be thrown in case the format.length will match one of the entries without matching the correct word. That is not the case anymore.
Sorry, something went wrong.
|
@starkwang I think if you reopen this and only remove the useless if statement, everyone would approve. |
Sorry, something went wrong.
Sorry, something went wrong.
One if statement in ECDH.getPublicKey is useless. This change
is to remove it and convert the following if-else-if chain to a switch
statement, which is more efficient.
Checklist
Affected core subsystem(s)
crypto