robot-name: Redesign for testability (#2114)

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,64 @@
 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();
+        let new_name = generate_name(rng, &mut used_names);
+        used_names.remove(&self.name);
+        self.name = new_name
     }
 }

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

@@ -7,6 +7,7 @@ edition = "2024"
 # The full list of available libraries is here:
 # https://github.com/exercism/rust-test-runner/blob/main/local-registry/Cargo.toml
 [dependencies]
+rand = "0.9"
 
 [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,10 @@
-use robot_name as robot;
+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 +20,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 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());
 }
 
 #[test]
 #[ignore]
-fn many_different_robots_have_different_names() {
-    use std::collections::HashSet;
-
-    // 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();
+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 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 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() {
+    let mut factory = RobotFactory::new();
+
+    let mut rng = deterministic_rng();
+    let mut robot_1 = factory.new_robot(&mut rng);
+    robot_1.reset(&mut rng);
+
+    let mut rng = deterministic_rng();
+    let mut robot_2 = factory.new_robot(&mut rng);
+    robot_2.reset(&mut rng);
+
+    assert_ne!(robot_1.name(), robot_2.name());
+}
+
+#[test]
+#[ignore]
+fn old_name_becomes_available_after_reset() {
+    let mut rng = deterministic_rng();
+    let mut factory = RobotFactory::new();
+    let mut robot = factory.new_robot(&mut rng);
+    let first_name = robot.name().to_string();
+    robot.reset(&mut rng); // cause first name to become available again
+    let mut rng = deterministic_rng(); // reset rng
+    let second_robot = factory.new_robot(&mut rng);
+    assert_eq!(second_robot.name(), first_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 {