First Rust Repo

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.

Second, you have this:

const DIGITS: &str = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
static DIGITS_BYTES: &[u8] = DIGITS.as_bytes();

As the str version is never used except for creating the bytes version, you can create a bytestring directly instead:

const DIGITS = b"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";

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.

7 Likes
  • You can create &[u8] directly with b"...".
    static DIGITS_BYTES: &[u8] = b"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
    
  • 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];
          }
      }
    

2 Likes

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.

https://doc.rust-lang.org/reference/expressions/range-expr.html#range-expressions

Edit: this should have been a reply to @dc0d, not to @kaj.

1 Like

$ 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();.

What am I missing?

On Mac:

$ rustc --version
rustc 1.63.0 (4b91a6ea7 2022-08-08)

$ cargo --version
cargo 1.63.0 (fd9c4297c 2022-07-01)

Output of cargo test:

$ 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

Update: Created this GitHub issue for cargo.

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.

1 Like