First Rust Repo

code review
#1

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).

#2

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
#3
  • 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
#4

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
#5

$ 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.

#6

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