improve(error-handling): return Result instead of panic

Instead of panicking return proper Results with custom error type.

This allows for better error messages and also increases testability, allowing for more comprehensive testing of validation errors.

Bring in test_case crate, which makes data-table driven tests possible and rewrite tests to take advantage of this.

Also fix a bug, where upper bound of the value range wasn't treated as valid speed option.

Author: AlexCzar

Cargo.lock

@@ -7,3 +7,6 @@ edition = "2018"
 [dependencies]
 getopts = "0.2"
 rusb = "0.8"
+
+[dev-dependencies]
+test-case = "1.1"

Cargo.toml

@@ -20,7 +20,7 @@ extern crate rusb;
 use std::fmt::Display;
 use std::fmt::Formatter;
 use std::num::ParseIntError;
-use std::ops::Range;
+use std::ops::RangeInclusive;
 use std::time::Duration;
 
 use getopts::{Matches, Options};
@@ -29,18 +29,22 @@ use rusb::{Device, GlobalContext};
 use crate::DeviceParam::{FanSpeed, PumpSpeed};
 
 fn main() {
+    if let Err(e) = run() {
+        eprintln!("Error: {}", e);
+        std::process::exit(1);
+    }
+}
+
+fn run<'e>() -> Result<(), CliValidationError<'e>> {
     let args: Vec<String> = std::env::args().collect();
     let program = args[0].clone();
     let opts = build_options();
     let opt_matches = match_opts(args, &opts);
 
     if opt_matches.opt_present("h") || no_options_present(&opt_matches) {
-        print_usage(program, opts)
+        Ok(print_usage(program, opts))
     } else {
-        match validate(opt_matches) {
-            Ok(params) => set_params(params),
-            Err(f) => eprintln!("Invalid options: {}", f),
-        }
+        validate(opt_matches).map(set_params)
     }
 }
 
@@ -54,30 +58,61 @@ fn set_params(params: Vec<DeviceParam>) {
     kraken.reattach();
 }
 
-fn validate(opt_matches: Matches) -> Result<Vec<DeviceParam>, ParseIntError> {
-    let range: Range<u8> = 10..100;
-
-    let mut params: Vec<DeviceParam> = Vec::new();
+#[derive(Debug)]
+struct CliValidationError<'e> {
+    value: String,
+    param: &'e str,
+    reason: String,
+}
 
-    if let Some(fan_speed) = opt_matches.opt_get("f")? {
-        assert!(
-            range.contains(&fan_speed),
-            "FANSPEED should be within range 10-100"
-        );
-        params.push(FanSpeed(fan_speed))
+impl Display for CliValidationError<'_> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "Value '{}' is not valid for parameter '{}': {}. Try --help.",
+            self.value, self.param, self.reason
+        )
     }
+}
 
-    if let Some(pump_speed) = opt_matches.opt_get("p")? {
-        assert!(
-            range.contains(&pump_speed),
-            "PUMPSPEED should be within range 10-100"
-        );
-        params.push(PumpSpeed(pump_speed))
-    }
+fn validate<'e>(opt_matches: Matches) -> Result<Vec<DeviceParam>, CliValidationError<'e>> {
+    let range = 10..=100;
 
+    let mut params: Vec<DeviceParam> = Vec::new();
+    parse_param(&opt_matches, &range, "fan-speed", FanSpeed)?.map(|dp| params.push(dp));
+    parse_param(&opt_matches, &range, "pump-speed", PumpSpeed)?.map(|dp| params.push(dp));
     Ok(params)
 }
 
+fn parse_param<'e>(
+    opt_matches: &Matches,
+    range: &RangeInclusive<u8>,
+    param: &'e str,
+    creator: fn(u8) -> DeviceParam,
+) -> Result<Option<DeviceParam>, CliValidationError<'e>> {
+    opt_matches
+        .opt_get(param)
+        .or_else(|f: ParseIntError| {
+            Err(CliValidationError {
+                param,
+                value: opt_matches.opt_str(param).unwrap(),
+                reason: f.to_string(),
+            })
+        })?
+        .map(|fan_speed| {
+            if range.contains(&fan_speed) {
+                Ok(Some(creator(fan_speed)))
+            } else {
+                Err(CliValidationError {
+                    param,
+                    value: fan_speed.to_string(),
+                    reason: "out of range".to_string(),
+                })
+            }
+        })
+        .unwrap_or_else(|| Ok(None))
+}
+
 fn no_options_present(opt_matches: &Matches) -> bool {
     !opt_matches.opt_present("f") && !opt_matches.opt_present("p")
 }
@@ -104,7 +139,7 @@ fn build_options() -> Options {
         "p",
         "pump-speed",
         "Pump speed in 10 - 100 range.",
-        "PUMPSEED",
+        "PUMPSPEED",
     );
     opts
 }
@@ -176,6 +211,7 @@ impl KrakenControl for Kraken {
     }
 }
 
+#[derive(Debug, PartialOrd, PartialEq)]
 enum DeviceParam {
     FanSpeed(u8),
     PumpSpeed(u8),

src/main.rs

@@ -14,37 +14,51 @@ See the License for the specific language governing permissions and
 limitations under the License.
  */
 
-#[cfg(test)]
-use super::*;
+use test_case::test_case;
 
-#[test]
-#[should_panic(expected = "FANSPEED should be within range 10-100")]
-fn fan_speed_should_fail_validation_if_value_less_than_range() {
-    let opts = build_options();
-    let command_line = vec!["krak", "-f9"];
-    opts.parse(command_line).map(|r| validate(r));
-}
+use super::*;
 
-#[test]
-#[should_panic(expected = "FANSPEED should be within range 10-100")]
-fn fan_speed_should_fail_validation_if_value_more_than_range() {
+#[test_case("--fan-speed", 9; "FANSPEED less than range min")]
+#[test_case("--fan-speed", 101; "FANSPEED more than range max")]
+#[test_case("--pump-speed", 9; "PUMPSPEED less than range min")]
+#[test_case("--pump-speed", 101; "PUMPSPEED more than range max")]
+fn should_fail_validation_if_speed_is_outside_of_range(test_param: &str, test_value: u8) {
     let opts = build_options();
-    let command_line = vec!["krak", "-f101"];
-    opts.parse(command_line).map(|r| validate(r));
+    let fanspeed = format!("{}={}", test_param, test_value);
+    let command_line = vec!["krak", fanspeed.as_str()];
+    let result = opts.parse(command_line).map(|r| validate(r)).unwrap();
+    assert!(result.is_err());
+    if let Err(CliValidationError {
+        value,
+        param,
+        reason,
+    }) = result
+    {
+        assert_eq!(test_value.to_string(), value);
+        assert_eq!("out of range", reason);
+        assert_eq!(param, param)
+    }
 }
 
-#[test]
-#[should_panic(expected = "PUMPSPEED should be within range 10-100")]
-fn pump_speed_should_fail_validation_if_value_less_than_range() {
+#[test_case("--fan-speed", 10, FanSpeed(10); "FANSPEED is range.min")]
+#[test_case("--fan-speed", 42, FanSpeed(42); "FANSPEED is inside range")]
+#[test_case("--fan-speed", 100, FanSpeed(100); "FANSPEED is range.max")]
+#[test_case("--pump-speed", 10, PumpSpeed(10); "PUMPSPEED is range.min")]
+#[test_case("--pump-speed", 42, PumpSpeed(42); "PUMPSPEED is inside range")]
+#[test_case("--pump-speed", 100, PumpSpeed(100); "PUMPSPEED is range.max")]
+fn should_pass_validation_if_speed_is_in_range(
+    test_param: &str,
+    test_value: u8,
+    test_opt: DeviceParam,
+) {
     let opts = build_options();
-    let command_line = vec!["krak", "-p9"];
-    opts.parse(command_line).map(|r| validate(r));
-}
+    let fanspeed = format!("{}={}", test_param, test_value);
+    let command_line = vec!["krak", fanspeed.as_str()];
+    let result = opts.parse(command_line).map(|r| validate(r)).unwrap();
 
-#[test]
-#[should_panic(expected = "PUMPSPEED should be within range 10-100")]
-fn pump_speed_should_fail_validation_if_value_more_than_range() {
-    let opts = build_options();
-    let command_line = vec!["krak", "-p101"];
-    opts.parse(command_line).map(|r| validate(r));
+    assert!(result.is_ok());
+    if let Ok(vec) = result {
+        assert_eq!(vec.len(), 1);
+        assert_eq!(vec.first().unwrap(), &test_opt);
+    }
 }