Merge pull request #1461 from CodeYourFuture/sprint-2-error-handling

Make Sprint 2 exercises throw on bad input, matching the Sprint 1 prep

Author: Liam310

Sprint-2/implement/contains.test.js

@@ -9,6 +9,9 @@ as the object contains a key of 'a'
 
 E.g. contains({a: 1, b: 2}, 'c') // returns false
 as the object doesn't contains a key of 'c'
+
+E.g. contains([1, 2, 3], 'a') throws Error("contains requires an object")
+as an array isn't an object
 */
 
 // Acceptance criteria:
@@ -30,6 +33,8 @@ test.todo("contains on empty object returns false");
 // When passed to contains with a non-existent property name
 // Then it should return false
 
-// Given invalid parameters like an array
+// Given a value that isn't an object - an array, a string, a number,
+// null, or no argument at all
 // When passed to contains
-// Then it should return false or throw an error
+// Then it should throw Error("contains requires an object")
+// (careful: typeof [] and typeof null are both "object")

Sprint-2/implement/tally.test.js

@@ -29,6 +29,6 @@ test.todo("tally on an empty array returns an empty object");
 // When passed to tally
 // Then it should return counts for each unique item
 
-// Given an invalid input like a string
+// Given an invalid input like a string, a number, or no argument at all
 // When passed to tally
-// Then it should throw an error
+// Then it should throw Error("tally requires an array")

Sprint-2/stretch/mode.js

@@ -1,22 +1,35 @@
-// You are given an implementation of calculateMode
+// You are given a working implementation of calculateMode.
+// Run the tests in mode.test.js before you start: they all pass.
 
-// calculateMode's implementation can be broken down into two stages:
+// calculateMode's implementation can be broken down into three stages:
 
-// Stage 1. One part of the code tracks the frequency of each value
-// Stage 2. The other part finds the value with the highest frequency
+// Stage 1. Check the input is a non-empty array of numbers, and throw if not
+// Stage 2. Track the frequency of each value
+// Stage 3. Find the value with the highest frequency
 
-// refactor calculateMode by splitting up the code
-// into smaller functions using the stages above
+// Refactor calculateMode by moving each stage into its own function,
+// then calling those functions from calculateMode.
+
+// The tests must still pass after your refactor. Run them again to check.
 
 function calculateMode(list) {
+  // check the input is a non-empty array of numbers
+  if (!Array.isArray(list)) {
+    throw new Error("calculateMode requires an array of numbers");
+  }
+  if (list.length === 0) {
+    throw new Error("calculateMode requires a non-empty array");
+  }
+  for (const item of list) {
+    if (typeof item !== "number") {
+      throw new Error("calculateMode requires an array of numbers");
+    }
+  }
+
   // track frequency of each value
   let freqs = new Map();
 
   for (let num of list) {
-    if (typeof num !== "number") {
-      continue;
-    }
-
     freqs.set(num, (freqs.get(num) || 0) + 1);
   }
 
@@ -30,7 +43,7 @@ function calculateMode(list) {
     }
   }
 
-  return maxFreq === 0 ? NaN : mode;
+  return mode;
 }
 
 module.exports = calculateMode;

Sprint-2/stretch/mode.test.js

@@ -1,3 +1,5 @@
+// Don't change this file. The tests check your refactored calculateMode still behaves the same.
+
 const calculateMode = require("./mode.js");
 
 // Acceptance criteria for calculateMode function
@@ -6,10 +8,15 @@ const calculateMode = require("./mode.js");
 // When calculateMode is called on the array
 // Then it should return the number that appears most frequently in the array
 
-// Example:
-// Given [2,4,1,2,3,2,1]
-// When calculateMode is called on [2,4,1,2,3,2,1]
-// Then it should return 2 */
+// E.g. calculateMode([2, 4, 1, 2, 3, 2, 1]) returns 2
+
+// Given an empty array
+// When calculateMode is called on the array
+// Then it should throw Error("calculateMode requires a non-empty array")
+
+// Given something that isn't an array of numbers, e.g. "banana" or [1, "2", 3]
+// When calculateMode is called on it
+// Then it should throw Error("calculateMode requires an array of numbers")
 
 describe("calculateMode()", () => {
   test("returns the most frequent number in an array", () => {
@@ -24,9 +31,27 @@ describe("calculateMode()", () => {
     expect(calculateMode(nums)).toEqual(2);
   });
 
-  test("ignores non-number values", () => {
-    const nums = [1, 3, "2", 2, 3, null];
+  test("throws when the array contains a non-number", () => {
+    expect(() => calculateMode([1, 3, "2", 2, 3, null])).toThrow(
+      new Error("calculateMode requires an array of numbers")
+    );
+  });
+
+  test("throws when given something that isn't an array", () => {
+    expect(() => calculateMode("banana")).toThrow(
+      new Error("calculateMode requires an array of numbers")
+    );
+  });
+
+  test("throws when given no argument", () => {
+    expect(() => calculateMode()).toThrow(
+      new Error("calculateMode requires an array of numbers")
+    );
+  });
 
-    expect(calculateMode(nums)).toEqual(3);
+  test("throws when given an empty array", () => {
+    expect(() => calculateMode([])).toThrow(
+      new Error("calculateMode requires a non-empty array")
+    );
   });
 });