#9 switch to absolute values rather than % when looking for spikes

src/main/scala/bad/robot/temperature/Barrier.scala

@@ -0,0 +1,10 @@
+package bad.robot.temperature
+
+import java.lang.Math._
+
+case class Barrier(degrees: Degrees) {
+
+  def breached(previous: Temperature, current: Temperature) = {
+    abs(current.celsius - previous.celsius) > degrees
+  }
+}

src/main/scala/bad/robot/temperature/ErrorOnTemperatureSpike.scala

@@ -1,38 +1,35 @@
 package bad.robot.temperature
 
-import java.lang.Math._
-
-import bad.robot.temperature.ErrorOnTemperatureSpike.DefaultSpikePercentage
-import bad.robot.temperature.PercentageDifference.percentageDifference
 import bad.robot.logging._
+import bad.robot.temperature.ErrorOnTemperatureSpike.DefaultBarrier
 
 import scala.collection.concurrent.TrieMap
 import scala.util.{Failure, Success, Try}
 import scalaz.{-\/, \/}
 
 object ErrorOnTemperatureSpike {
 
-  private val DefaultSpikePercentage = 25
+  private val DefaultBarrier = Barrier(5)
 
   /**
     * @param delegate delegate writer
-    * @return a [[TemperatureWriter]] that will produce an error (left disjunction) when a spike over n percent
+    * @return a [[TemperatureWriter]] that will produce an error (left disjunction) when a spike over n degrees
     *         is detected or pass through to the delegate if the system property `avoid.spikes` is not set.
     *         
-    *         n is configured via a system property `avoid.spikes` and must be greater than 10 and less than 100
+    *         n is configured via a system property `avoid.spikes` and must be greater than 1 and less than 100
     */
   def apply(delegate: TemperatureWriter): TemperatureWriter = {
-    sys.props.get("avoid.spikes").map(spike => {
-      val percentage = toInt(spike).getOrElse(DefaultSpikePercentage)
-      Log.info(s"Temperature spikes greater than +/-$percentage% will not be recorded")
-      new ErrorOnTemperatureSpike(delegate, percentage)
+    sys.props.get("avoid.spikes").map(value => {
+      val barrier = toBarrier(value)
+      Log.info(s"Temperature spikes greater than +/-${barrier.degrees} °C will not be recorded")
+      new ErrorOnTemperatureSpike(delegate, barrier)
     }).getOrElse(delegate)
   }
 
-  private def toInt(string: String) = Try(string.toInt) match {
-    case Success(value) if value >= 10 && value <= 100 => Some(value)
-    case Failure(_)                                    => None
-    case _                                             => None
+  private def toBarrier(string: String) = Try(string.toInt) match {
+    case Success(value) if value >= 1 && value <= 100 => Barrier(value)
+    case Failure(_)                                   => DefaultBarrier
+    case _                                            => DefaultBarrier
   }
 
 }
@@ -46,16 +43,16 @@ object ErrorOnTemperatureSpike {
   * However, as we know that for every host, at most one call will be made every 30 seconds, there is no risk on
   * concurrent access for a particular sensor (the key to the cache).
   */
-class ErrorOnTemperatureSpike(delegate: TemperatureWriter, percentageSpike: Int = DefaultSpikePercentage) extends TemperatureWriter {
+class ErrorOnTemperatureSpike(delegate: TemperatureWriter, barrier: Barrier = DefaultBarrier) extends TemperatureWriter {
 
   private val temperatures: TrieMap[String, Temperature] = TrieMap()
 
   def write(measurement: Measurement): Error \/ Unit = {
 
     val spiked = measurement.temperatures.flatMap(current => {
       temperatures.get(current.name) match {
-        case Some(previous) if spikeBetween(current, previous) => Some(Spike(current.name, previous, current.temperature))
-        case _                                                 => None
+        case Some(previous) if breached(current, previous) => Some(Spike(current.name, previous, current.temperature))
+        case _                                             => None
       }
     })
 
@@ -67,9 +64,8 @@ class ErrorOnTemperatureSpike(delegate: TemperatureWriter, percentageSpike: Int
     }
   }
 
-  private def spikeBetween(reading: SensorReading, previous: Temperature) = {
-    val difference = percentageDifference(previous.celsius, reading.temperature.celsius)
-    abs(difference) >= percentageSpike || difference.isNaN
+  private def breached(reading: SensorReading, previous: Temperature) = {
+    barrier.breached(previous, reading.temperature)
   }
 
 }

src/main/scala/bad/robot/temperature/PercentageDifference.scala

@@ -19,4 +19,4 @@ object PercentageDifference {
     case _                                    => BigDecimal(value).setScale(2, RoundingMode.HALF_UP).toDouble
   }
 
-}
+}

src/main/scala/bad/robot/temperature/package.scala

@@ -14,6 +14,8 @@ import scalaz.syntax.std.either._
 
 package object temperature {
 
+  type Degrees = Int
+
   // NB not implicit as this seems to clash with http4s implicits that are kicking around
   def jsonDecoder[A: Decoder]: EntityDecoder[IO, A] = jsonOf[IO, A]
   def jsonEncoder[A: Encoder]: EntityEncoder[IO, A] = jsonEncoderWithPrinterOf(spaces2PlatformSpecific)

src/test/scala/bad/robot/temperature/BarrierTest.scala

@@ -0,0 +1,37 @@
+package bad.robot.temperature
+
+import org.specs2.mutable.Specification
+
+import scala.Double._
+
+class BarrierTest extends Specification {
+
+  "No breach" >> {
+    val barrier = Barrier(5)
+    barrier.breached(Temperature(10.23), Temperature(11.22)) must_== false
+    barrier.breached(Temperature(-10.23), Temperature(-10.23)) must_== false
+    barrier.breached(Temperature(0), Temperature(0)) must_== false
+    barrier.breached(Temperature(23.12), Temperature(23.12)) must_== false
+    barrier.breached(Temperature(23.12), Temperature(24.11)) must_== false
+  }
+
+  "Positive breach" >> {
+    val barrier = Barrier(1)
+    barrier.breached(Temperature(10.23), Temperature(11.24)) must_== true
+    barrier.breached(Temperature(10.24), Temperature(11.23)) must_== false
+  }
+
+  "Negative breach" >> {
+    val barrier = Barrier(1)
+    barrier.breached(Temperature(-10.23), Temperature(-11.24)) must_== true
+    barrier.breached(Temperature(-10.24), Temperature(-11.23)) must_== false
+  }
+
+  "NaN" >> {
+    val barrier = Barrier(1)
+    barrier.breached(Temperature(-10.23), Temperature(NaN)) must_== false
+    barrier.breached(Temperature(NaN), Temperature(10.23)) must_== false
+    barrier.breached(Temperature(NaN), Temperature(NaN)) must_== false
+  }
+
+}

src/test/scala/bad/robot/temperature/ErrorOnTemperatureSpikeTest.scala

@@ -88,6 +88,22 @@ class ErrorOnTemperatureSpikeTest extends Specification {
       Measurement(Host("example", None), Seconds(3), List(SensorReading("A", Temperature(21.6))))
     )
   }
+
+  "Negative spikes values (single sensor), example from production" >> {
+    val delegate = new StubWriter
+    val writer = new ErrorOnTemperatureSpike(delegate, Barrier(1))
+    writer.write(Measurement(Host("example", None), Seconds(1), List(SensorReading("A", Temperature(0.5)))))
+    writer.write(Measurement(Host("example", None), Seconds(2), List(SensorReading("A", Temperature(0.5)))))
+    writer.write(Measurement(Host("example", None), Seconds(3), List(SensorReading("A", Temperature(0.4))))) 
+    writer.write(Measurement(Host("example", None), Seconds(4), List(SensorReading("A", Temperature(-0.7))))) must be_-\/.like(SensorError)
+    writer.write(Measurement(Host("example", None), Seconds(5), List(SensorReading("A", Temperature(0.4)))))
+    delegate.temperatures must_== List(
+      Measurement(Host("example", None), Seconds(1), List(SensorReading("A", Temperature(0.5)))),
+      Measurement(Host("example", None), Seconds(2), List(SensorReading("A", Temperature(0.5)))),
+      Measurement(Host("example", None), Seconds(3), List(SensorReading("A", Temperature(0.4)))),
+      Measurement(Host("example", None), Seconds(5), List(SensorReading("A", Temperature(0.4))))
+    )
+  }
 
   "NaN (32.625 - 0.0 / 0.0 * 100 is NaN)" >> {
     val delegate = new StubWriter
@@ -139,30 +155,24 @@ class ErrorOnTemperatureSpikeTest extends Specification {
     }
   }
 
-  "What happens with NaN" >> {
+  "What happens with NaN (let it through)" >> {
     val delegate = new StubWriter
     val writer = new ErrorOnTemperatureSpike(delegate)
     writer.write(Measurement(Host("example", None), Seconds(1), List(SensorReading("A", Temperature(21.1)))))
     writer.write(Measurement(Host("example", None), Seconds(2), List(SensorReading("A", Temperature(21.6)))))
-    writer.write(Measurement(Host("example", None), Seconds(3), List(SensorReading("A", Temperature(NaN))))) must be_-\/.like {
-      case e: SensorSpikeError => e.message must_==
-        """An unexpected spike was encountered on:
-          | sensor(s)             : A
-          | previous temperatures : 21.6 °C
-          | spiked temperatures   : NaN °C
-          |""".stripMargin
-    }
+    writer.write(Measurement(Host("example", None), Seconds(3), List(SensorReading("A", Temperature(NaN))))) must be_\/- 
     writer.write(Measurement(Host("example", None), Seconds(4), List(SensorReading("A", Temperature(21.8)))))
     delegate.temperatures must containAllOf(List(
       Measurement(Host("example", None), Seconds(1), List(SensorReading("A", Temperature(21.1)))),
       Measurement(Host("example", None), Seconds(2), List(SensorReading("A", Temperature(21.6)))),
+//      Measurement(Host("example", None), Seconds(3), List(SensorReading("A", Temperature(NaN)))), // can't do equality check on NaN
       Measurement(Host("example", None), Seconds(4), List(SensorReading("A", Temperature(21.8))))
     ))
   }
 
   "Toggle the use based on system property" >> {
     ErrorOnTemperatureSpike(new StubWriter()) must haveClass[StubWriter]
-    sys.props += ("avoid.spikes" -> "30")
+    sys.props += ("avoid.spikes" -> "6")
     ErrorOnTemperatureSpike(new StubWriter()) must haveClass[ErrorOnTemperatureSpike]
   }