diff --git a/exercises/react/example.rs b/exercises/react/example.rs index f3ac5c60c..0df1c7c54 100644 --- a/exercises/react/example.rs +++ b/exercises/react/example.rs @@ -3,6 +3,18 @@ use std::collections::HashMap; pub type CellID = usize; pub type CallbackID = usize; +#[derive(Debug, PartialEq)] +pub enum SetValueError { + NonexistentCell, + ComputeCell, +} + +#[derive(Debug, PartialEq)] +pub enum RemoveCallbackError { + NonexistentCell, + NonexistentCallback, +} + struct Cell<'a, T: Copy> { value: T, last_value: T, @@ -46,11 +58,13 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> { self.cells.len() - 1 } - pub fn create_compute T + 'a>(&mut self, dependencies: &[CellID], compute_func: F) -> Result { + pub fn create_compute T + 'a>(&mut self, dependencies: &[CellID], compute_func: F) -> Result { // Check all dependencies' validity before modifying any of them, // so that we don't perform an incorrect partial write. - if !dependencies.iter().all(|&id| id < self.cells.len()) { - return Err("Nonexistent input"); + for &dep in dependencies { + if dep >= self.cells.len() { + return Err(dep); + } } let new_id = self.cells.len(); for &id in dependencies { @@ -66,16 +80,16 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> { self.cells.get(id).map(|c| c.value) } - pub fn set_value(&mut self, id: CellID, new_value: T) -> Result<(), &'static str> { + pub fn set_value(&mut self, id: CellID, new_value: T) -> Result<(), SetValueError> { match self.cells.get_mut(id) { Some(c) => match c.cell_type { CellType::Input => { c.value = new_value; Ok(c.dependents.clone()) }, - CellType::Compute(_, _) => Err("Can't set compute cell value directly"), + CellType::Compute(_, _) => Err(SetValueError::ComputeCell), }, - None => Err("Can't set nonexistent cell"), + None => Err(SetValueError::NonexistentCell), }.map(|deps| { for &d in deps.iter() { self.update_dependent(d); @@ -88,24 +102,21 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> { }) } - pub fn add_callback () + 'a>(&mut self, id: CellID, callback: F) -> Result { - match self.cells.get_mut(id) { - Some(c) => { - c.callbacks_issued += 1; - c.callbacks.insert(c.callbacks_issued, Box::new(callback)); - Ok(c.callbacks_issued) - }, - None => Err("Can't add callback to nonexistent cell"), - } + pub fn add_callback () + 'a>(&mut self, id: CellID, callback: F) -> Option { + self.cells.get_mut(id).map(|c| { + c.callbacks_issued += 1; + c.callbacks.insert(c.callbacks_issued, Box::new(callback)); + c.callbacks_issued + }) } - pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), &'static str> { + pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), RemoveCallbackError> { match self.cells.get_mut(cell) { Some(c) => match c.callbacks.remove(&callback) { Some(_) => Ok(()), - None => Err("Can't remove nonexistent callback"), + None => Err(RemoveCallbackError::NonexistentCallback), }, - None => Err("Can't remove callback from nonexistent cell"), + None => Err(RemoveCallbackError::NonexistentCell), } } diff --git a/exercises/react/src/lib.rs b/exercises/react/src/lib.rs index 4ec956768..600840ad2 100644 --- a/exercises/react/src/lib.rs +++ b/exercises/react/src/lib.rs @@ -5,6 +5,18 @@ pub type CellID = (); pub type CallbackID = (); +#[derive(Debug, PartialEq)] +pub enum SetValueError { + NonexistentCell, + ComputeCell, +} + +#[derive(Debug, PartialEq)] +pub enum RemoveCallbackError { + NonexistentCell, + NonexistentCallback, +} + pub struct Reactor { // Just so that the compiler doesn't complain about an unused type parameter. // You probably want to delete this field. @@ -28,12 +40,14 @@ impl Reactor { // You do not need to reject compute functions that expect more arguments than there are // dependencies (how would you check for this, anyway?). // - // Return an Err (and you can change the error type) if any dependency doesn't exist. + // If any dependency doesn't exist, returns an Err with that nonexistent dependency. + // (If multiple dependencies do not exist, exactly which one is returned is not defined and + // will not be tested) // // Notice that there is no way to *remove* a cell. // This means that you may assume, without checking, that if the dependencies exist at creation // time they will continue to exist as long as the Reactor exists. - pub fn create_compute T>(&mut self, dependencies: &[CellID], compute_func: F) -> Result { + pub fn create_compute T>(&mut self, dependencies: &[CellID], compute_func: F) -> Result { unimplemented!() } @@ -50,20 +64,22 @@ impl Reactor { // Sets the value of the specified input cell. // - // Return an Err (and you can change the error type) if the cell does not exist, or the - // specified cell is a compute cell, since compute cells cannot have their values directly set. + // Returns an Err if either: + // * the cell does not exist + // * the specified cell is a compute cell, since compute cells cannot have their values + // directly set. // // Similarly, you may wonder about `get_mut(&mut self, id: CellID) -> Option<&mut Cell>`, with // a `set_value(&mut self, new_value: T)` method on `Cell`. // // As before, that turned out to add too much extra complexity. - pub fn set_value(&mut self, id: CellID, new_value: T) -> Result<(), ()> { + pub fn set_value(&mut self, id: CellID, new_value: T) -> Result<(), SetValueError> { unimplemented!() } // Adds a callback to the specified compute cell. // - // Return an Err (and you can change the error type) if the cell does not exist. + // Returns the ID of the just-added callback, or None if the cell doesn't exist. // // Callbacks on input cells will not be tested. // @@ -73,17 +89,16 @@ impl Reactor { // * Exactly once if the compute cell's value changed as a result of the set_value call. // The value passed to the callback should be the final value of the compute cell after the // set_value call. - pub fn add_callback ()>(&mut self, id: CellID, callback: F) -> Result { + pub fn add_callback ()>(&mut self, id: CellID, callback: F) -> Option { unimplemented!() } // Removes the specified callback, using an ID returned from add_callback. // - // Return an Err (and you can change the error type) if either the cell or callback - // does not exist. + // Returns an Err if either the cell or callback does not exist. // // A removed callback should no longer be called. - pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), ()> { + pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), RemoveCallbackError> { unimplemented!() } } diff --git a/exercises/react/tests/react.rs b/exercises/react/tests/react.rs index f9ff2ec04..ddc38b0b3 100644 --- a/exercises/react/tests/react.rs +++ b/exercises/react/tests/react.rs @@ -2,6 +2,39 @@ extern crate react; use react::*; +struct CallbackRecorder { + // Note that this `Cell` is https://doc.rust-lang.org/std/cell/ + // a mechanism to allow internal mutability, + // distinct from the cells (input cells, compute cells) in the reactor + value: std::cell::Cell>, +} + +/// A CallbackRecorder helps tests whether callbacks get called correctly. +/// You'll see it used in tests that deal with callbacks. +/// The names should be descriptive enough so that the tests make sense, +/// so it's not necessary to fully understand the implementation, +/// though you are welcome to. +impl CallbackRecorder { + fn new() -> Self { + CallbackRecorder { + value: std::cell::Cell::new(None), + } + } + + fn expect_to_have_been_called_with(&self, v: isize) { + assert_ne!(self.value.get(), None, "Callback was not called, but should have been"); + assert_eq!(self.value.replace(None), Some(v), "Callback was called with incorrect value"); + } + + fn expect_not_to_have_been_called(&self) { + assert_eq!(self.value.get(), None, "Callback was called, but should not have been"); + } + + fn callback_called(&self, v: isize) { + assert_eq!(self.value.replace(Some(v)), None, "Callback was called too many times; can't be called with {}", v); + } +} + #[test] fn input_cells_have_a_value() { let mut reactor = Reactor::new(); @@ -23,7 +56,7 @@ fn an_input_cells_value_can_be_set() { fn error_setting_a_nonexistent_input_cell() { let mut dummy_reactor = Reactor::new(); let input = dummy_reactor.create_input(1); - assert!(Reactor::new().set_value(input, 0).is_err()); + assert_eq!(Reactor::new().set_value(input, 0), Err(SetValueError::NonexistentCell)); } #[test] @@ -50,7 +83,7 @@ fn compute_cells_take_inputs_in_the_right_order() { fn error_creating_compute_cell_if_input_doesnt_exist() { let mut dummy_reactor = Reactor::new(); let input = dummy_reactor.create_input(1); - assert!(Reactor::new().create_compute(&[input], |_| 0).is_err()); + assert_eq!(Reactor::new().create_compute(&[input], |_| 0), Err(input)); } #[test] @@ -61,7 +94,7 @@ fn do_not_break_cell_if_creating_compute_cell_with_valid_and_invalid_input() { let dummy_cell = dummy_reactor.create_input(2); let mut reactor = Reactor::new(); let input = reactor.create_input(1); - assert!(reactor.create_compute(&[input, dummy_cell], |_| 0).is_err()); + assert_eq!(reactor.create_compute(&[input, dummy_cell], |_| 0), Err(dummy_cell)); assert!(reactor.set_value(input, 5).is_ok()); assert_eq!(reactor.value(input), Some(5)); } @@ -96,23 +129,19 @@ fn error_setting_a_compute_cell() { let mut reactor = Reactor::new(); let input = reactor.create_input(1); let output = reactor.create_compute(&[input], |_| 0).unwrap(); - assert!(reactor.set_value(output, 3).is_err()); + assert_eq!(reactor.set_value(output, 3), Err(SetValueError::ComputeCell)); } #[test] #[ignore] fn compute_cells_fire_callbacks() { - // This is a bit awkward, but the closure mutably borrows `values`. - // So we have to end its borrow by taking reactor out of scope. - let mut values = Vec::new(); - { - let mut reactor = Reactor::new(); - let input = reactor.create_input(1); - let output = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); - assert!(reactor.add_callback(output, |v| values.push(v)).is_ok()); - assert!(reactor.set_value(input, 3).is_ok()); - } - assert_eq!(values, vec![4]); + let cb = CallbackRecorder::new(); + let mut reactor = Reactor::new(); + let input = reactor.create_input(1); + let output = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); + assert!(reactor.add_callback(output, |v| cb.callback_called(v)).is_some()); + assert!(reactor.set_value(input, 3).is_ok()); + cb.expect_to_have_been_called_with(4); } #[test] @@ -121,101 +150,102 @@ fn error_adding_callback_to_nonexistent_cell() { let mut dummy_reactor = Reactor::new(); let input = dummy_reactor.create_input(1); let output = dummy_reactor.create_compute(&[input], |_| 0).unwrap(); - assert!(Reactor::new().add_callback(output, |_: usize| println!("hi")).is_err()); + assert_eq!(Reactor::new().add_callback(output, |_: usize| println!("hi")), None); } #[test] #[ignore] fn callbacks_only_fire_on_change() { - let mut values = Vec::new(); - { - let mut reactor = Reactor::new(); - let input = reactor.create_input(1); - let output = reactor.create_compute(&[input], |v| if v[0] < 3 { 111 } else { 222 }).unwrap(); - assert!(reactor.add_callback(output, |v| values.push(v)).is_ok()); - assert!(reactor.set_value(input, 2).is_ok()); - assert!(reactor.set_value(input, 4).is_ok()); - } - assert_eq!(values, vec![222]); + let cb = CallbackRecorder::new(); + let mut reactor = Reactor::new(); + let input = reactor.create_input(1); + let output = reactor.create_compute(&[input], |v| if v[0] < 3 { 111 } else { 222 }).unwrap(); + assert!(reactor.add_callback(output, |v| cb.callback_called(v)).is_some()); + + assert!(reactor.set_value(input, 2).is_ok()); + cb.expect_not_to_have_been_called(); + assert!(reactor.set_value(input, 4).is_ok()); + cb.expect_to_have_been_called_with(222); } #[test] #[ignore] fn callbacks_can_be_added_and_removed() { - let mut values1 = Vec::new(); - let mut values2 = Vec::new(); - let mut values3 = Vec::new(); - { - let mut reactor = Reactor::new(); - let input = reactor.create_input(11); - let output = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); - let callback = reactor.add_callback(output, |v| values1.push(v)).unwrap(); - assert!(reactor.add_callback(output, |v| values2.push(v)).is_ok()); - assert!(reactor.set_value(input, 31).is_ok()); - assert!(reactor.remove_callback(output, callback).is_ok()); - assert!(reactor.add_callback(output, |v| values3.push(v)).is_ok()); - assert!(reactor.set_value(input, 41).is_ok()); - } - assert_eq!(values1, vec![32]); - assert_eq!(values2, vec![32, 42]); - assert_eq!(values3, vec![42]); + let cb1 = CallbackRecorder::new(); + let cb2 = CallbackRecorder::new(); + let cb3 = CallbackRecorder::new(); + + let mut reactor = Reactor::new(); + let input = reactor.create_input(11); + let output = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); + + let callback = reactor.add_callback(output, |v| cb1.callback_called(v)).unwrap(); + assert!(reactor.add_callback(output, |v| cb2.callback_called(v)).is_some()); + + assert!(reactor.set_value(input, 31).is_ok()); + cb1.expect_to_have_been_called_with(32); + cb2.expect_to_have_been_called_with(32); + + assert!(reactor.remove_callback(output, callback).is_ok()); + assert!(reactor.add_callback(output, |v| cb3.callback_called(v)).is_some()); + + assert!(reactor.set_value(input, 41).is_ok()); + cb1.expect_not_to_have_been_called(); + cb2.expect_to_have_been_called_with(42); + cb3.expect_to_have_been_called_with(42); } #[test] #[ignore] fn removing_a_callback_multiple_times_doesnt_interfere_with_other_callbacks() { - let mut values1 = Vec::new(); - let mut values2 = Vec::new(); - { - let mut reactor = Reactor::new(); - let input = reactor.create_input(1); - let output = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); - let callback = reactor.add_callback(output, |v| values1.push(v)).unwrap(); - assert!(reactor.add_callback(output, |v| values2.push(v)).is_ok()); - // We want the first remove to be Ok, but we don't care about the others. - assert!(reactor.remove_callback(output, callback).is_ok()); - for _ in 1..5 { - assert!(reactor.remove_callback(output, callback).is_err()); - } - assert!(reactor.set_value(input, 2).is_ok()); + let cb1 = CallbackRecorder::new(); + let cb2 = CallbackRecorder::new(); + + let mut reactor = Reactor::new(); + let input = reactor.create_input(1); + let output = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); + let callback = reactor.add_callback(output, |v| cb1.callback_called(v)).unwrap(); + assert!(reactor.add_callback(output, |v| cb2.callback_called(v)).is_some()); + // We want the first remove to be Ok, but the others should be errors. + assert!(reactor.remove_callback(output, callback).is_ok()); + for _ in 1..5 { + assert_eq!(reactor.remove_callback(output, callback), Err(RemoveCallbackError::NonexistentCallback)); } - assert_eq!(values1, Vec::new()); - assert_eq!(values2, vec![3]); + + assert!(reactor.set_value(input, 2).is_ok()); + cb1.expect_not_to_have_been_called(); + cb2.expect_to_have_been_called_with(3); } #[test] #[ignore] fn callbacks_should_only_be_called_once_even_if_multiple_dependencies_change() { - let mut values = Vec::new(); - { - let mut reactor = Reactor::new(); - let input = reactor.create_input(1); - let plus_one = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); - let minus_one1 = reactor.create_compute(&[input], |v| v[0] - 1).unwrap(); - let minus_one2 = reactor.create_compute(&[minus_one1], |v| v[0] - 1).unwrap(); - let output = reactor.create_compute(&[plus_one, minus_one2], |v| v[0] * v[1]).unwrap(); - assert!(reactor.add_callback(output, |v| values.push(v)).is_ok()); - assert!(reactor.set_value(input, 4).is_ok()); - } - assert_eq!(values, vec![10]); + let cb = CallbackRecorder::new(); + let mut reactor = Reactor::new(); + let input = reactor.create_input(1); + let plus_one = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); + let minus_one1 = reactor.create_compute(&[input], |v| v[0] - 1).unwrap(); + let minus_one2 = reactor.create_compute(&[minus_one1], |v| v[0] - 1).unwrap(); + let output = reactor.create_compute(&[plus_one, minus_one2], |v| v[0] * v[1]).unwrap(); + assert!(reactor.add_callback(output, |v| cb.callback_called(v)).is_some()); + assert!(reactor.set_value(input, 4).is_ok()); + cb.expect_to_have_been_called_with(10); } #[test] #[ignore] fn callbacks_should_not_be_called_if_dependencies_change_but_output_value_doesnt_change() { - let mut values = Vec::new(); - { - let mut reactor = Reactor::new(); - let input = reactor.create_input(1); - let plus_one = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); - let minus_one = reactor.create_compute(&[input], |v| v[0] - 1).unwrap(); - let always_two = reactor.create_compute(&[plus_one, minus_one], |v| v[0] - v[1]).unwrap(); - assert!(reactor.add_callback(always_two, |v| values.push(v)).is_ok()); - for i in 2..5 { - assert!(reactor.set_value(input, i).is_ok()); - } + let cb = CallbackRecorder::new(); + let mut reactor = Reactor::new(); + let input = reactor.create_input(1); + let plus_one = reactor.create_compute(&[input], |v| v[0] + 1).unwrap(); + let minus_one = reactor.create_compute(&[input], |v| v[0] - 1).unwrap(); + let always_two = reactor.create_compute(&[plus_one, minus_one], |v| v[0] - v[1]).unwrap(); + assert!(reactor.add_callback(always_two, |v| cb.callback_called(v)).is_some()); + for i in 2..5 { + assert!(reactor.set_value(input, i).is_ok()); + cb.expect_not_to_have_been_called(); } - assert_eq!(values, Vec::new()); } #[test]