Improve padWithZeroes implementation and add tests #180
Conversation
rekmarks
force-pushed
the
improve-padWithZeroes
branch
from
4e0f659
to
a8f7907
Compare
rekmarks
force-pushed
the
improve-padWithZeroes
branch
from
7e43330
to
cf45227
Compare
| function padWithZeroes(number: string, length: number): string { | ||
| let myString = `${number}`; | ||
| while (myString.length < length) { | ||
| myString = `0${myString}`; |
There was a problem hiding this comment.
This computes a new string and stores it in memory length - number.length times.
src/index.ts
Outdated
| * @returns The input string front-padded with zeroes, or the original string | ||
| * if it was already greater than or equal to to the target length. | ||
| */ | ||
| export function padWithZeroes(hexString: string, targetLength: number): string { |
There was a problem hiding this comment.
Gotta say, I'm not too keen on adding more stuff to this API 🤔 We've already exposed an annoying amount of what would otherwise be internal functions.
I think it would be fine to test this via concatSig, remove the tests, and keep this private.
There was a problem hiding this comment.
Either that or delete this altogether and use String.prototype.padStart
There was a problem hiding this comment.
TIL padStart is a thing! I'm definitely going to use that.
|
@Gudahtt I refactored it to use |
| export function padWithZeroes(hexString: string, targetLength: number): string { | ||
| if (hexString !== '' && !/^[a-f0-9]+$/iu.test(hexString)) { | ||
| throw new Error( | ||
| `Expected an unprefixed hex string. Received: ${hexString}`, |
There was a problem hiding this comment.
The old function didn't include this check. It would coerce anything it was given into a string. I'm pretty confident it was only ever called with a hex string though, because in the one place it's called, it's given the result of a Buffer.prototype.toString('hex') call. So I'd guess this doesn't change behaviour.
There was a problem hiding this comment.
Yeah, my thinking was exactly that it's non-breaking for current usage, and we now also won't accidentally shoot ourselves in the foot in the future.
src/utils.test.ts
Outdated
| expect(padWithZeroes(input, 4)).toStrictEqual(`0000`); | ||
| }); | ||
|
|
||
| it('returns a string longer than or equal to the target length without modifying it', function () { |
There was a problem hiding this comment.
Nit: might be nice to test the "longer than" case, which isn't covered here.
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
|
Okay, I added a couple more test cases, including the one you suggested. I also added input validation for the target length. |
The
padWithZeroesimplementation was inefficient, but most of all, ugly. This PR makes it more efficient, adds some basic input validation, and unit tests.