From 0b18446c0c96af58eff4d6419e5757f2e4a571f6 Mon Sep 17 00:00:00 2001
From: Jason Peacock <jason@jasonpeacock.com>
Date: Sat, 27 Oct 2018 16:51:32 -0700
Subject: [PATCH] Addressed review feedback

---
 src/hyperfine/export/markdown.rs | 42 ++++++++++--------------------
 src/hyperfine/format.rs          | 44 +++++---------------------------
 2 files changed, 19 insertions(+), 67 deletions(-)

diff --git a/src/hyperfine/export/markdown.rs b/src/hyperfine/export/markdown.rs
index 8ef449d..948b424 100644
--- a/src/hyperfine/export/markdown.rs
+++ b/src/hyperfine/export/markdown.rs
@@ -5,23 +5,18 @@ use hyperfine::types::BenchmarkResult;
 
 use std::io::Result;
 
-macro_rules! TABLE_HEADER {
-    ($unit:expr) => {
-        format!("| Command | Mean [{unit}] | Min…Max [{unit}] |\n|:---|---:|---:|\n", unit=$unit) };
-}
-
 #[derive(Default)]
 pub struct MarkdownExporter {}
 
 impl Exporter for MarkdownExporter {
     fn serialize(&self, results: &Vec<BenchmarkResult>) -> Result<Vec<u8>> {
-        // Default to `Second`.
-        let mut unit = Unit::Second;
-
-        if !results.is_empty() {
+        let unit = if let Some(first_result) = results.first() {
             // Use the first BenchmarkResult entry to determine the unit for all entries.
-            unit = format_duration_value(results[0].mean, None).1;
-        }
+            format_duration_value(first_result.mean, None).1
+        } else {
+            // Default to `Second`.
+            Unit::Second
+        };
 
         let mut destination = start_table(unit);
 
@@ -33,8 +28,12 @@ impl Exporter for MarkdownExporter {
     }
 }
 
+fn table_header(unit_short_name: String) -> String {
+    format!("| Command | Mean [{unit}] | Min…Max [{unit}] |\n|:---|---:|---:|\n", unit=unit_short_name)
+}
+
 fn start_table(unit: Unit) -> Vec<u8> {
-    TABLE_HEADER!(unit.short_name()).bytes().collect()
+    table_header(unit.short_name()).bytes().collect()
 }
 
 fn add_table_row(dest: &mut Vec<u8>, entry: &BenchmarkResult, unit: Unit) {
@@ -95,7 +94,7 @@ fn test_markdown_format_ms() {
 "{}\
 | `sleep 0.1` | 105.7 ± 1.6 | 102.3…108.0 |
 | `sleep 2` | 2005.0 ± 2.0 | 2002.0…2008.0 |
-", TABLE_HEADER!("ms"));
+", table_header("ms".to_string()));
 
     assert_eq!(formatted_expected, formatted);
 }
@@ -136,22 +135,7 @@ fn test_markdown_format_s() {
 "{}\
 | `sleep 2` | 2.005 ± 0.002 | 2.002…2.008 |
 | `sleep 0.1` | 0.106 ± 0.002 | 0.102…0.108 |
-", TABLE_HEADER!("s"));
-
-    assert_eq!(formatted_expected, formatted);
-}
-
-/// An empty list of benchmark results will only include the table header
-/// in the markdown output, using the default `Seconds` unit.
-#[test]
-fn test_markdown_format_empty_results() {
-    let exporter = MarkdownExporter::default();
-
-    let timing_results = vec![];
-
-    let formatted = String::from_utf8(exporter.serialize(&timing_results).unwrap()).unwrap();
-
-    let formatted_expected = TABLE_HEADER!("s");
+", table_header("s".to_string()));
 
     assert_eq!(formatted_expected, formatted);
 }
diff --git a/src/hyperfine/format.rs b/src/hyperfine/format.rs
index cccfe97..5aba921 100644
--- a/src/hyperfine/format.rs
+++ b/src/hyperfine/format.rs
@@ -15,21 +15,11 @@ impl Unit {
         }
     }
 
-    /// The multiplier value to convert from `from_unit` to the Unit.
-    pub fn multiplier_from(&self, from_unit: Unit) -> f64 {
-        match (*self, from_unit) {
-            (Unit::Second, Unit::Second) => 1.0,
-            (Unit::MilliSecond, Unit::MilliSecond) => 1.0,
-            (Unit::Second, Unit::MilliSecond) => 1e-3,
-            (Unit::MilliSecond, Unit::Second) => 1e3,
-        }
-    }
-
     /// Returns the Second value formatted for the Unit.
     pub fn format(&self, value: Second) -> String {
         match *self {
-            Unit::Second => format!("{:.3}", value * self.multiplier_from(Unit::Second)),
-            Unit::MilliSecond => format!("{:.1}", value * self.multiplier_from(Unit::Second)),
+            Unit::Second => format!("{:.3}", value),
+            Unit::MilliSecond => format!("{:.1}", value * 1e3),
         }
     }
 }
@@ -51,24 +41,11 @@ pub fn format_duration_unit(duration: Second, unit: Option<Unit>) -> (String, Un
 
 /// Like `format_duration`, but returns the target unit as well.
 pub fn format_duration_value(duration: Second, unit: Option<Unit>) -> (String, Unit) {
-
-    // Default to `Second` until proven otherwise.
-    let mut duration_unit = Unit::Second;
-
-    match unit {
-        Some(unit_option) => {
-            // Use user-supplied unit.
-            duration_unit = unit_option;
-        },
-        None => {
-            if duration < 1.0 {
-                // It's a small value, use `Millisecond` instead.
-                duration_unit = Unit::MilliSecond;
-            }
-        },
+    if (duration < 1.0 && unit.is_none()) || unit == Some(Unit::MilliSecond) {
+        (Unit::MilliSecond.format(duration), Unit::MilliSecond)
+    } else {
+        (Unit::Second.format(duration), Unit::Second)
     }
-
-    (duration_unit.format(duration), duration_unit)
 }
 
 #[test]
@@ -77,15 +54,6 @@ fn test_unit_short_name() {
     assert_eq!("ms", Unit::MilliSecond.short_name());
 }
 
-#[test]
-fn test_unit_multiplier_from() {
-    assert_eq!(1.0, Unit::Second.multiplier_from(Unit::Second));
-    assert_eq!(1.0, Unit::MilliSecond.multiplier_from(Unit::MilliSecond));
-
-    assert_eq!(0.001, Unit::Second.multiplier_from(Unit::MilliSecond));
-    assert_eq!(1000.0, Unit::MilliSecond.multiplier_from(Unit::Second));
-}
-
 // Note - the values are rounded when formatted.
 #[test]
 fn test_unit_format() {