Merge pull request #464 from petertseng/react-errs

react: return domain-specific errors

Author: petertseng

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,11 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
         self.cells.len() - 1
     }
 
-    pub fn create_compute<F: Fn(&[T]) -> T + 'a>(&mut self, dependencies: &[CellID], compute_func: F) -> Result<CellID, &'static str> {
+    pub fn create_compute<F: Fn(&[T]) -> T + 'a>(&mut self, dependencies: &[CellID], compute_func: F) -> Result<CellID, CellID> {
         // 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");
+        if let Some(&invalid) = dependencies.iter().find(|&dep| *dep >= self.cells.len()) {
+            return Err(invalid);
         }
         let new_id = self.cells.len();
         for &id in dependencies {
@@ -66,16 +78,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 +100,21 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
         })
     }
 
-    pub fn add_callback<F: FnMut(T) -> () + 'a>(&mut self, id: CellID, callback: F) -> Result<CallbackID, &'static str> {
-        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<F: FnMut(T) -> () + 'a>(&mut self, id: CellID, callback: F) -> Option<CallbackID> {
+        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),
         }
     }
 

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<T> {
     // 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 <T: Copy + PartialEq> Reactor<T> {
     // 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<F: Fn(&[T]) -> T>(&mut self, dependencies: &[CellID], compute_func: F) -> Result<CellID, ()> {
+    pub fn create_compute<F: Fn(&[T]) -> T>(&mut self, dependencies: &[CellID], compute_func: F) -> Result<CellID, CellID> {
         unimplemented!()
     }
 
@@ -50,20 +64,22 @@ impl <T: Copy + PartialEq> Reactor<T> {
 
     // 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 <T: Copy + PartialEq> Reactor<T> {
     // * 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<F: FnMut(T) -> ()>(&mut self, id: CellID, callback: F) -> Result<CallbackID, ()> {
+    pub fn add_callback<F: FnMut(T) -> ()>(&mut self, id: CellID, callback: F) -> Option<CallbackID> {
         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!()
     }
 }

exercises/react/tests/react.rs

@@ -23,7 +23,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 +50,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 +61,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,7 +96,7 @@ 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));
 }
 
 /// A CallbackRecorder helps tests whether callbacks get called correctly.
@@ -139,7 +139,7 @@ fn compute_cells_fire_callbacks() {
     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_ok());
+    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);
 }
@@ -150,7 +150,7 @@ 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]
@@ -160,7 +160,7 @@ fn callbacks_only_fire_on_change() {
     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_ok());
+    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();
@@ -180,14 +180,14 @@ fn callbacks_can_be_added_and_removed() {
     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_ok());
+    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_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();
@@ -205,11 +205,11 @@ fn removing_a_callback_multiple_times_doesnt_interfere_with_other_callbacks() {
     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_ok());
-    // We want the first remove to be Ok, but we don't care about the others.
+    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!(reactor.remove_callback(output, callback).is_err());
+        assert_eq!(reactor.remove_callback(output, callback), Err(RemoveCallbackError::NonexistentCallback));
     }
 
     assert!(reactor.set_value(input, 2).is_ok());
@@ -227,7 +227,7 @@ fn callbacks_should_only_be_called_once_even_if_multiple_dependencies_change() {
     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_ok());
+    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);
 }
@@ -241,7 +241,7 @@ fn callbacks_should_not_be_called_if_dependencies_change_but_output_value_doesnt
     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_ok());
+    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();