robot-name: Redesign for testability

The previous exercise design had two fundamental flaws wrt. testability:

- The source of randomness was not injected by the tests. It was
  expected that users would initialize an RNG on their own. This lead to
  users simply not using any randomness at all.

- The fact that names were required to be _globally_ unique meant that
  there was no way to test for deterministic behavior, given a PRNG with
  a stable seed.

These two problems are solved by injecting an RNG from the tests and by
narrowing the requirement of unique names to a single "robot factory".

Motivated by this discussion on the forum:
https://forum.exercism.org/t/rust-track-check-predictability-of-names-in-robot-name-exercise/20077

The function `RobotFactory::new_robot` takes a `&mut self`, even
though it could take a `&self` as well. `&self` would work, because
the robot factory needs shared ownership & interior mutability anyway
to track robot names generated with `Robot::reset`. However, that
would slightly interfere with the TTD flow of this exercise. If
`RobotFactory::new_robot` takes a `&self`, users will be forced to use
interior mutability right away, even though it's unclear why that is
even necessary until they get around to implementing `Robot::reset`.

Author: senekor

exercises/practice/robot-name/.docs/instructions.append.md

@@ -22,6 +22,7 @@
     "pminten",
     "razielgn",
     "rofrol",
+    "senekor",
     "spazm",
     "stringparser",
     "workingjubilee",

exercises/practice/robot-name/.meta/Cargo-example.toml

@@ -1,52 +1,62 @@
 use std::{
     collections::HashSet,
-    sync::{LazyLock, Mutex},
+    sync::{Arc, Mutex},
 };
 
-use rand::{Rng, thread_rng};
+use rand::Rng;
 
-static NAMES: LazyLock<Mutex<HashSet<String>>> = LazyLock::new(|| Mutex::new(HashSet::new()));
-
-pub struct Robot {
-    name: String,
-}
-
-fn generate_name() -> String {
+fn generate_name<R: Rng>(rng: &mut R, used_names: &mut HashSet<String>) -> String {
     loop {
         let mut s = String::with_capacity(5);
-        static LETTERS: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZ";
-        static NUMBERS: &[u8] = b"0123456789";
         for _ in 0..2 {
-            s.push(*thread_rng().choose(LETTERS).unwrap() as char);
+            s.push(rng.random_range('A'..='Z'));
         }
         for _ in 0..3 {
-            s.push(*thread_rng().choose(NUMBERS).unwrap() as char);
+            s.push(rng.random_range('0'..='9'));
         }
-
-        if NAMES.lock().unwrap().insert(s.clone()) {
+        if used_names.insert(s.clone()) {
             return s;
         }
     }
 }
 
-impl Robot {
-    pub fn new() -> Robot {
-        Robot {
-            name: generate_name(),
+/// A `RobotFactory` is responsible for ensuring that all robots produced by
+/// it have a unique name. Robots from different factories can have the same
+/// name.
+pub struct RobotFactory {
+    used_names: Arc<Mutex<HashSet<String>>>,
+}
+
+pub struct Robot {
+    used_names: Arc<Mutex<HashSet<String>>>,
+    name: String,
+}
+
+impl RobotFactory {
+    pub fn new() -> Self {
+        Self {
+            used_names: Arc::new(Mutex::new(HashSet::new())),
         }
     }
 
-    pub fn name(&self) -> &str {
-        &self.name[..]
+    pub fn new_robot<R: Rng>(&mut self, rng: &mut R) -> Robot {
+        let mut guard = self.used_names.lock().unwrap();
+        Robot {
+            used_names: Arc::clone(&self.used_names),
+            name: generate_name(rng, &mut guard),
+        }
     }
+}
 
-    pub fn reset_name(&mut self) {
-        self.name = generate_name();
+impl Robot {
+    pub fn name(&self) -> &str {
+        &self.name
     }
-}
 
-impl Default for Robot {
-    fn default() -> Self {
-        Self::new()
+    // When a robot is reset, its factory must still ensure that there are no
+    // name conflicts with other robots from the same factory.
+    pub fn reset<R: Rng>(&mut self, rng: &mut R) {
+        let mut used_names = self.used_names.lock().unwrap();
+        self.name = generate_name(rng, &mut used_names);
     }
 }

exercises/practice/robot-name/.meta/config.json

@@ -7,6 +7,9 @@ edition = "2024"
 # The full list of available libraries is here:
 # https://github.com/exercism/rust-test-runner/blob/main/local-registry/Cargo.toml
 [dependencies]
+# We use a wild-card specifier to minimize the risk of breaking existing
+# solutions in case the version in the test runner is updated.
+rand = "*"
 
 [lints.clippy]
 new_without_default = "allow"

exercises/practice/robot-name/.meta/example.rs

@@ -1,15 +1,28 @@
+use rand::Rng;
+
+/// A `RobotFactory` is responsible for ensuring that all robots produced by
+/// it have a unique name. Robots from different factories can have the same
+/// name.
+pub struct RobotFactory;
+
 pub struct Robot;
 
-impl Robot {
+impl RobotFactory {
     pub fn new() -> Self {
-        todo!("Construct a new Robot struct.");
+        todo!("Create a new robot factory")
     }
 
+    pub fn new_robot<R: Rng>(&mut self, _rng: &mut R) -> Robot {
+        todo!("Create a new robot with a unique name")
+    }
+}
+
+impl Robot {
     pub fn name(&self) -> &str {
-        todo!("Return the reference to the robot's name.");
+        todo!("Return a reference to the robot's name");
     }
 
-    pub fn reset_name(&mut self) {
-        todo!("Assign a new unique name to the robot.");
+    pub fn reset<R: Rng>(&mut self, _rng: &mut R) {
+        todo!("Assign a new unique name to the robot");
     }
 }

exercises/practice/robot-name/Cargo.toml

@@ -1,4 +1,12 @@
-use robot_name as robot;
+use std::collections::HashSet;
+
+use rand::SeedableRng as _;
+use rand::rngs::SmallRng;
+use robot_name::*;
+
+fn deterministic_rng() -> SmallRng {
+    SmallRng::seed_from_u64(0)
+}
 
 fn assert_name_matches_pattern(n: &str) {
     assert!(n.len() == 5, "name is exactly 5 characters long");
@@ -14,46 +22,86 @@ fn assert_name_matches_pattern(n: &str) {
 
 #[test]
 fn name_should_match_expected_pattern() {
-    let r = robot::Robot::new();
-    assert_name_matches_pattern(r.name());
+    let mut rng = deterministic_rng();
+    let mut factory = RobotFactory::new();
+    let robot = factory.new_robot(&mut rng);
+    assert_name_matches_pattern(robot.name());
 }
 
 #[test]
 #[ignore]
-fn different_robots_have_different_names() {
-    let r1 = robot::Robot::new();
-    let r2 = robot::Robot::new();
-    assert_ne!(r1.name(), r2.name(), "Robot names should be different");
+fn factory_prevents_name_collisions() {
+    let mut factory = RobotFactory::new();
+    let robot_1 = factory.new_robot(&mut deterministic_rng());
+    let robot_2 = factory.new_robot(&mut deterministic_rng());
+    assert_ne!(robot_1.name(), robot_2.name());
 }
 
 #[test]
 #[ignore]
-fn many_different_robots_have_different_names() {
-    use std::collections::HashSet;
+fn robot_name_depends_on_rng() {
+    let mut rng = deterministic_rng();
+    let robot_1 = RobotFactory::new().new_robot(&mut rng);
+    let robot_2 = RobotFactory::new().new_robot(&mut rng);
+    assert_ne!(robot_1.name(), robot_2.name());
+}
 
-    // In 3,529 random robot names, there is ~99.99% chance of a name collision
-    let vec: Vec<_> = (0..3529).map(|_| robot::Robot::new()).collect();
-    let set: HashSet<_> = vec.iter().map(|robot| robot.name()).collect();
+#[test]
+#[ignore]
+fn robot_name_only_depends_on_rng() {
+    let robot_1 = RobotFactory::new().new_robot(&mut deterministic_rng());
+    let robot_2 = RobotFactory::new().new_robot(&mut deterministic_rng());
+    assert_eq!(robot_1.name(), robot_2.name());
+}
 
-    let number_of_collisions = vec.len() - set.len();
-    assert_eq!(number_of_collisions, 0);
+#[test]
+#[ignore]
+fn many_different_robots_have_different_names() {
+    // In 3,529 random robot names, there is ~99.99% chance of a name collision
+    let mut rng = deterministic_rng();
+    let mut factory = RobotFactory::new();
+    let robots: Vec<_> = (0..3529).map(|_| factory.new_robot(&mut rng)).collect();
+    let mut set = HashSet::new();
+    assert!(robots.iter().all(|robot| set.insert(robot.name())));
 }
 
 #[test]
 #[ignore]
 fn new_name_should_match_expected_pattern() {
-    let mut r = robot::Robot::new();
-    assert_name_matches_pattern(r.name());
-    r.reset_name();
-    assert_name_matches_pattern(r.name());
+    let mut rng = deterministic_rng();
+    let mut factory = RobotFactory::new();
+    let mut robot = factory.new_robot(&mut rng);
+    assert_name_matches_pattern(robot.name());
+    robot.reset(&mut rng);
+    assert_name_matches_pattern(robot.name());
 }
 
 #[test]
 #[ignore]
 fn new_name_is_different_from_old_name() {
-    let mut r = robot::Robot::new();
-    let n1 = r.name().to_string();
-    r.reset_name();
-    let n2 = r.name().to_string();
-    assert_ne!(n1, n2, "Robot name should change when reset");
+    let mut rng = deterministic_rng();
+    let mut factory = RobotFactory::new();
+    let mut robot = factory.new_robot(&mut rng);
+    let name_1 = robot.name().to_string();
+    robot.reset(&mut rng);
+    let name_2 = robot.name().to_string();
+    assert_ne!(name_1, name_2, "Robot name should change when reset");
+}
+
+#[test]
+#[ignore]
+fn factory_prevents_name_collision_despite_reset() {
+    // To keep the same probablity as the first test with many robots, we
+    // generate 3,529 robots and reset their names, then generate another 3,529
+    // robots and check if there are collisions across these two groups.
+    let mut rng = deterministic_rng();
+    let mut factory = RobotFactory::new();
+    let mut reset_robots: Vec<_> = (0..3529).map(|_| factory.new_robot(&mut rng)).collect();
+    for robot in &mut reset_robots {
+        robot.reset(&mut rng);
+    }
+    let mut set = HashSet::new();
+    assert!(reset_robots.iter().all(|robot| set.insert(robot.name())));
+
+    assert!((0..3529).all(|_| !set.contains(factory.new_robot(&mut rng).name())));
 }

exercises/practice/robot-name/src/lib.rs

@@ -56,6 +56,7 @@ static EXCEPTIONS: &[&str] = &[
     "roman-numerals",     // std::fmt::Formatter is not Debug
     "simple-linked-list", // has generics
     "sublist",            // has generics
+    "robot-name",         // has generics
 ];
 
 fn line_is_not_a_comment(line: &&str) -> bool {