Conversation
| } | ||
|
|
||
| return buf || bytesToUuid(rnds); | ||
| return bytesToUuid(rnds); |
There was a problem hiding this comment.
Gotta return buf if it's passed in. (This should have been caught by unit tests. Can you add a test to check this?)
| return bytesToUuid(rnds); | |
| return buf || bytesToUuid(rnds); |
There was a problem hiding this comment.
@broofa sure, I can add test for this case.
But the code can be seen that the buffer is returned after it is filled:
https://github.com/uuidjs/uuid/pull/435/files#diff-8988f35cbc1e6b5ea7f9cb52a4bfa0f2R26
There was a problem hiding this comment.
@broofa there is already tests for this case:
https://github.com/uuidjs/uuid/blob/master/test/unit/v4.test.js#L46-L73
There was a problem hiding this comment.
@awwit I just realized that we currently do two things: fill the passed buffer and return it. But we only test for filling, not for the returned buffer.
Since we don't know if anybody relies on the fact, that our methods fill the provided buffer and return it, it would be a good opportunity to add another test case that test for the return value when a buffer is passed.
ctavan
left a comment
There was a problem hiding this comment.
Nice work and sorry for the delay in review!
I think we have an opportunity here to add a test case here, see comments.
src/v4.js
Outdated
|
|
||
| function v4(options, buf, offset) { | ||
| const i = (buf && offset) || 0; | ||
| const start = (buf && offset) || 0; |
There was a problem hiding this comment.
Define this variable only in the if(buf) scope below, it's not needed outside. It then simplifies to offset | 0.
An alternative would be to use default arguments. We'd have to check how efficient the bable'd code is then though (in terms of bundlesize).
| const i = (buf && offset) || 0; | ||
| const start = (buf && offset) || 0; | ||
|
|
||
| if (typeof options === 'string') { |
| } | ||
|
|
||
| return buf || bytesToUuid(rnds); | ||
| return bytesToUuid(rnds); |
There was a problem hiding this comment.
@awwit I just realized that we currently do two things: fill the passed buffer and return it. But we only test for filling, not for the returned buffer.
Since we don't know if anybody relies on the fact, that our methods fill the provided buffer and return it, it would be a good opportunity to add another test case that test for the return value when a buffer is passed.
* Simplify the v3/v5 tests for the buffer api * Add test cases to v1/v3/v5 to ensure that if passing a buffer as second parameter that buffer is not only filled but also returned. For v4 this has already been added in #435.
* Simplify the v3/v5 tests for the buffer api * Add test cases to v1/v3/v5 to ensure that if passing a buffer as second parameter that buffer is not only filled but also returned. For v4 this has already been added in #435.
Performance is increased by using a single
rnds8buffer to generate random numbers (as in browser version).Changes affect only the
v1andv4.Benchmark master:
Benchmark this branch: