nuid is a port of nuid in Go (It's a unique identifier generator used by NATS).
I am looking for code review and feedback to learn rust.
What has been done wrong?
What could have been done in a better way? (Rustier?)
Note: I have no opinion regarding the nuid package itself - whether it is good or bad. Only non-cryptographic random number generators are used in my implementation (for now).
I don't really know nuid, but I took a look at your code and have some comments. First of, it looks basically reasonable, no real problems that I see.
But here's some optional nitpicking. You have this:
MIN_INC + fastrand::i64(1..(MAX_INC - MIN_INC)),
I think it would be clearer like this:
fastrand::i64(MIN_INC..MAX_INC),
Note that this is not really the same thing. Your code can return MIN_INC + 1 but not MIN_INC. If that is your intention, I would probably think it less surprising to increase the value of the MIN_INC constant instead, but you could also use (MIN_INC+1)..MAX_INC as the range.
Here, DIGITS is &[u8] (thanks to the b prefix), and so DIGITS_BYTES is not needed.
Also, you seem to do CONSTANT as usize in a few places where the constant is i64 (but have a positive value). I would consider declaring those constants as usize in the first place instead.
Might as well keep consts that are always or almost always cast to usize as usize from the start.
const BASE: usize = 62;
const PRE_LEN: usize = 12;
const SEQ_LEN: usize = 10;
const TOTAL_LEN: usize = PRE_LEN + SEQ_LEN;
// ... much removal of `as usize`, one new `as i64`
Side note: Is this supposed to work when usize is not a 64-bit value? Perhaps guard against that if not.
Since you have fixed lengths, you can use arrays instead of Vec.
pub struct NUID {
pre: [u8; PRE_LEN],
// ...
let mut b = [0; TOTAL_LEN];
You can replace your assignment loop with a method call.
b[..PRE_LEN].copy_from_slice(&self.pre);
You can often replace indexing with iteration.
let mut l = self.seq as usize;
for target in b[PRE_LEN..].iter_mut().rev() {
*target = DIGITS_BYTES[l % BASE];
l /= BASE;
}
// ...
fn randomize_prefix(&mut self) {
for target in &mut self.pre {
let rnd_idx = (fastrand::u8(..) as i64) % BASE as i64;
*target = DIGITS_BYTES[rnd_idx as usize];
}
}
Be aware that a .. b creates a range that includes the first endpoint and excludes the second. Thus, if the endpoints are integers, this range's maximum will be MAX_INC - 1, not MAX_INC. If you want to include the second endpoint, use MIN_INC..=MAX_INC.
$ cargo test (and rust analyzer in vscode) say this line is an unused import: use std::collections::HashMap;. While HashMap is used in this line: let mut map: HashMap<String, u8> = HashMap::new();.
$ cargo test
warning: unused import: `std::collections::HashMap`
--> src/lib.rs:2:5
|
2 | use std::collections::HashMap;
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: `nuid` (lib) generated 1 warning
Finished test [unoptimized + debuginfo] target(s) in 0.07s
Running unittests src/lib.rs (target/debug/deps/nuid-61219c4efa23785a)
running 5 tests
test tests::test_basic_uniqueness ... ignored
test tests::test_digits ... ok
test tests::test_nuid_rollover ... ok
test tests::test_guid_len ... ok
test tests::test_proper_prefix ... ok
test result: ok. 4 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 0.29s
Doc-tests nuid
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Usually, if you have an import that you only use in your tests, you place it inside the mod tests block, next to the use super::*;. This means that the import will only be part of the file when tests are enabled, and in this case it's sufficient to prevent the unused_imports lint. I'm not sure why it fires even when you compile with cargo test; I suspect it has to do with there being both a library and binary target for unit tests, only one of which adds cfg(test) blocks.