From 692c9cc8a4e34d9300734311f5423335e681d269 Mon Sep 17 00:00:00 2001
From: Peter Tseng <pht24@cornell.edu>
Date: Thu, 22 Feb 2018 18:39:38 -0800
Subject: [PATCH 1/6] react: Express callback expectations precisely after each
 `set_value`

**Problem statement**:

Consider a test with two `set_value` calls and which expects that a
callback has, ultimately, been called with the two values, one for each
`set_value`.

The tests currently do not check that one value was added during each
`set_value` call. For all we know, maybe an implementation:

* magically manages to predict the future and calls the callback twice
  on the first `set_value` call, with the correct value.
* calls the callback zero times on the first `set_value` call, but twice
  on the second `set_value` call.

To more precisely define the `set_value` expectations, this commit uses
a `Cell`-based implementation to test callbacks.
---
 exercises/react/tests/react.rs | 186 +++++++++++++++++++--------------
 1 file changed, 108 insertions(+), 78 deletions(-)

diff --git a/exercises/react/tests/react.rs b/exercises/react/tests/react.rs
index f9ff2ec04..31e8ff68c 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<Option<isize>>,
+}
+
+/// 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();
@@ -102,17 +135,13 @@ fn error_setting_a_compute_cell() {
 #[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_ok());
+    assert!(reactor.set_value(input, 3).is_ok());
+    cb.expect_to_have_been_called_with(4);
 }
 
 #[test]
@@ -127,95 +156,96 @@ fn error_adding_callback_to_nonexistent_cell() {
 #[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_ok());
+
+    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_ok());
+
+    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.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_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_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_ok());
+    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_ok());
+    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]

From 2cf3f4fd1ea3ce58b8a4cefae6a339df180eecae Mon Sep 17 00:00:00 2001
From: Peter Tseng <pht24@cornell.edu>
Date: Fri, 16 Mar 2018 01:06:39 +0000
Subject: [PATCH 2/6] react: return erroneous CellID from create_compute

Instead of encouraging `()` as an Err type, we'd prefer to encourage
informative errors. In this case, there is only one failure mode (so we
do not need a separate error type) but there is a parameter (exactly
which cell is invalid) so we'll keep Result to designate the invalid
cell.

As discussed in https://github.com/exercism/rust/issues/444
A subtask of https://github.com/exercism/rust/issues/462
---
 exercises/react/example.rs     | 8 +++++---
 exercises/react/src/lib.rs     | 6 ++++--
 exercises/react/tests/react.rs | 4 ++--
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/exercises/react/example.rs b/exercises/react/example.rs
index f3ac5c60c..c68a33dfb 100644
--- a/exercises/react/example.rs
+++ b/exercises/react/example.rs
@@ -46,11 +46,13 @@ 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");
+        for &dep in dependencies {
+            if dep >= self.cells.len() {
+                return Err(dep);
+            }
         }
         let new_id = self.cells.len();
         for &id in dependencies {
diff --git a/exercises/react/src/lib.rs b/exercises/react/src/lib.rs
index 4ec956768..b2fd952c7 100644
--- a/exercises/react/src/lib.rs
+++ b/exercises/react/src/lib.rs
@@ -28,12 +28,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!()
     }
 
diff --git a/exercises/react/tests/react.rs b/exercises/react/tests/react.rs
index 31e8ff68c..6028e5594 100644
--- a/exercises/react/tests/react.rs
+++ b/exercises/react/tests/react.rs
@@ -83,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]
@@ -94,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));
 }

From c547a9b0064f51e3f3ad760220581d8957108ba9 Mon Sep 17 00:00:00 2001
From: Peter Tseng <pht24@cornell.edu>
Date: Fri, 16 Mar 2018 01:13:32 +0000
Subject: [PATCH 3/6] react: return Option from add_callback

Instead of encouraging `()` as an Err type, we'd prefer to encourage
informative errors. In this case, there is only one failure mode and it
requires no extra information (there is only one cell specified) so
Option is sufficient.

As discussed in https://github.com/exercism/rust/issues/444
A subtask of https://github.com/exercism/rust/issues/462
---
 exercises/react/example.rs     |  6 +++---
 exercises/react/src/lib.rs     |  4 ++--
 exercises/react/tests/react.rs | 16 ++++++++--------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/exercises/react/example.rs b/exercises/react/example.rs
index c68a33dfb..f9ef52c8c 100644
--- a/exercises/react/example.rs
+++ b/exercises/react/example.rs
@@ -90,14 +90,14 @@ 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> {
+    pub fn add_callback<F: FnMut(T) -> () + 'a>(&mut self, id: CellID, callback: F) -> Option<CallbackID> {
         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)
+                Some(c.callbacks_issued)
             },
-            None => Err("Can't add callback to nonexistent cell"),
+            None => None,
         }
     }
 
diff --git a/exercises/react/src/lib.rs b/exercises/react/src/lib.rs
index b2fd952c7..47080d9d6 100644
--- a/exercises/react/src/lib.rs
+++ b/exercises/react/src/lib.rs
@@ -65,7 +65,7 @@ impl <T: Copy + PartialEq> Reactor<T> {
 
     // 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.
     //
@@ -75,7 +75,7 @@ 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!()
     }
 
diff --git a/exercises/react/tests/react.rs b/exercises/react/tests/react.rs
index 6028e5594..c43dcbad7 100644
--- a/exercises/react/tests/react.rs
+++ b/exercises/react/tests/react.rs
@@ -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,7 +205,7 @@ 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());
+    assert!(reactor.add_callback(output, |v| cb2.callback_called(v)).is_some());
     // 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 {
@@ -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();

From 0902b8e6776d8a873e0ba67ffdc2aba774c0ffbc Mon Sep 17 00:00:00 2001
From: Peter Tseng <pht24@cornell.edu>
Date: Fri, 16 Mar 2018 01:14:50 +0000
Subject: [PATCH 4/6] react: Use Option::map in add_callback example

Now that it's simplified to use Option, we can simply map into the
`get_mut`.
---
 exercises/react/example.rs | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/exercises/react/example.rs b/exercises/react/example.rs
index f9ef52c8c..63588e8c9 100644
--- a/exercises/react/example.rs
+++ b/exercises/react/example.rs
@@ -91,14 +91,11 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
     }
 
     pub fn add_callback<F: FnMut(T) -> () + 'a>(&mut self, id: CellID, callback: F) -> Option<CallbackID> {
-        match self.cells.get_mut(id) {
-            Some(c) => {
-                c.callbacks_issued += 1;
-                c.callbacks.insert(c.callbacks_issued, Box::new(callback));
-                Some(c.callbacks_issued)
-            },
-            None => None,
-        }
+        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> {

From a7e043d4935006f99faac2e63d676364862d9d58 Mon Sep 17 00:00:00 2001
From: Peter Tseng <pht24@cornell.edu>
Date: Fri, 16 Mar 2018 01:21:28 +0000
Subject: [PATCH 5/6] react: return domain-specific error from set_value

Instead of encouraging `()` as an Err type, we'd prefer to encourage
informative errors. In this case, there are multiple failure modes so we
need to have a specific enum for it.

As discussed in https://github.com/exercism/rust/issues/444
A subtask of https://github.com/exercism/rust/issues/462
---
 exercises/react/example.rs     | 12 +++++++++---
 exercises/react/src/lib.rs     | 14 +++++++++++---
 exercises/react/tests/react.rs |  4 ++--
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/exercises/react/example.rs b/exercises/react/example.rs
index 63588e8c9..272d784f6 100644
--- a/exercises/react/example.rs
+++ b/exercises/react/example.rs
@@ -3,6 +3,12 @@ use std::collections::HashMap;
 pub type CellID = usize;
 pub type CallbackID = usize;
 
+#[derive(Debug, PartialEq)]
+pub enum SetValueError {
+    NonexistentCell,
+    ComputeCell,
+}
+
 struct Cell<'a, T: Copy> {
     value: T,
     last_value: T,
@@ -68,16 +74,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);
diff --git a/exercises/react/src/lib.rs b/exercises/react/src/lib.rs
index 47080d9d6..8c6bf3a24 100644
--- a/exercises/react/src/lib.rs
+++ b/exercises/react/src/lib.rs
@@ -5,6 +5,12 @@
 pub type CellID = ();
 pub type CallbackID = ();
 
+#[derive(Debug, PartialEq)]
+pub enum SetValueError {
+    NonexistentCell,
+    ComputeCell,
+}
+
 pub struct Reactor<T> {
     // Just so that the compiler doesn't complain about an unused type parameter.
     // You probably want to delete this field.
@@ -52,14 +58,16 @@ 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!()
     }
 
diff --git a/exercises/react/tests/react.rs b/exercises/react/tests/react.rs
index c43dcbad7..c3b7ae610 100644
--- a/exercises/react/tests/react.rs
+++ b/exercises/react/tests/react.rs
@@ -56,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]
@@ -129,7 +129,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));
 }
 
 #[test]

From 8d1835a5edd73e5b18c619293e8c809f6e64fd3c Mon Sep 17 00:00:00 2001
From: Peter Tseng <pht24@cornell.edu>
Date: Fri, 16 Mar 2018 01:41:34 +0000
Subject: [PATCH 6/6] react: return domain-specific error from remove_callback

Instead of encouraging `()` as an Err type, we'd prefer to encourage
informative errors. In this case, there are multiple failure modes so we
need to have a specific enum for it.

As discussed in https://github.com/exercism/rust/issues/444
A subtask of https://github.com/exercism/rust/issues/462
---
 exercises/react/example.rs     | 12 +++++++++---
 exercises/react/src/lib.rs     | 11 ++++++++---
 exercises/react/tests/react.rs |  4 ++--
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/exercises/react/example.rs b/exercises/react/example.rs
index 272d784f6..0df1c7c54 100644
--- a/exercises/react/example.rs
+++ b/exercises/react/example.rs
@@ -9,6 +9,12 @@ pub enum SetValueError {
     ComputeCell,
 }
 
+#[derive(Debug, PartialEq)]
+pub enum RemoveCallbackError {
+    NonexistentCell,
+    NonexistentCallback,
+}
+
 struct Cell<'a, T: Copy> {
     value: T,
     last_value: T,
@@ -104,13 +110,13 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
         })
     }
 
-    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 8c6bf3a24..600840ad2 100644
--- a/exercises/react/src/lib.rs
+++ b/exercises/react/src/lib.rs
@@ -11,6 +11,12 @@ pub enum SetValueError {
     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.
@@ -89,11 +95,10 @@ impl <T: Copy + PartialEq> Reactor<T> {
 
     // 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 c3b7ae610..ddc38b0b3 100644
--- a/exercises/react/tests/react.rs
+++ b/exercises/react/tests/react.rs
@@ -206,10 +206,10 @@ fn removing_a_callback_multiple_times_doesnt_interfere_with_other_callbacks() {
     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 we don't care about the others.
+    // 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());