crypto: Remove useless if statement and convert if-else-if chain #14911
Conversation
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.
nodejs-github-bot
added
the
crypto
| 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.
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.
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.
| 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.
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
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.
| 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.
|
@starkwang I think if you reopen this and only remove the useless if statement, everyone would approve. |
One if statement in
ECDH.getPublicKeyis useless. This changeis to remove it and convert the following
if-else-ifchain to aswitchstatement, which is more efficient.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
crypto