Manchester | 26-ITP-Sep | Mathew Shaibu | Sprint 2 | Module data group sprint 2 by MattCarolus10 · Pull Request #1499 · CodeYourFuture/Module-Data-Groups

MattCarolus10 · 2026-11-06T22:09:53Z

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Task code

CYF-1083

Changelist

I completed debugging task, implemented test and created function and interpreted codes

…perty existence

… pairs

…t comments

…ecks and comments

…comments and whitespace

…onents

Liam310 · 2026-11-10T08:49:09Z

-  // implementation here
+function createLookup(countryCurrencyPairs) {
+  const lookup = {};
+  for (const [country, currency] of countryCurrencyPairs) {


Neat use of array destructuring!

Liam310 · 2026-11-10T08:50:34Z

+test("creates a country currency code lookup for multiple codes", () => {
+  expect(
+    createLookup([
+      ["US", "USD"],
+      ["CA", "CAD"],
+    ])
+  ).toEqual({ US: "USD", CA: "CAD" });
+});


One test is rarely enough to comprehensively cover all behaviours of a function. Can you think of what some other good test cases might be?

Liam310 · 2026-11-10T08:51:42Z

+    const k = decodeURIComponent(key.split("+").join(" "));
+    const v = decodeURIComponent(value.split("+").join(" "));


Single-letter variable names are something we generally want to avoid - what would be more descriptive choices for these variable? What do k and v actually represent?

Liam310 · 2026-11-10T09:03:13Z

+test("tally on an empty array returns an empty object", () => {
+  expect(tally([])).toEqual({});
+});

-// Given a function called tally
-// When passed an array of items
-// Then it should return an object containing the count for each unique item
+test("tally counts duplicate items", () => {
+  expect(tally(["a", "a", "a"])).toEqual({ a: 3 });
+  expect(tally(["a", "a", "b", "c"])).toEqual({ a: 2, b: 1, c: 1 });
+});


When building up tests with TDD, it isn't always as direct as "1 test per acceptance criteria". We want to think about the different behaviours of this function and increasing complexity of the inputs in small increments.

Your second test here contains two assertions which have notably different inputs. One of them has the same letter 3 times and that's it. The other one has multiple letters. You're checking two different things here:

Can the function count multiple occurrences of the same letter?

Can the function count occurrences of different letters?

Those are distinct behaviours, so they should be separate tests.

Liam310 · 2026-11-10T09:03:41Z

+  expect(() => tally("banana")).toThrow("tally requires an array");
+  expect(() => tally(42)).toThrow("tally requires an array");
+  expect(() => tally()).toThrow("tally requires an array");
+});


A great example of using multiple assertions that are all checking the same behaviour - a non-array input.

Liam310 · 2026-11-10T09:08:00Z

+const invert = require("./invert.js");
+
+test("swaps key with values", () => {
+  expect(invert({ x: 10, y: 20 })).toEqual({ 10: "x", 20: "y" });
+});
+
+test("handles single entry", () => {
+  expect(invert({ a: 1 })).toEqual({ 1: "a" });
+});
+
+test("handles an empty object", () => {
+  expect(invert({})).toEqual({});
+});


Extra credit for adding a test suite when we didn't ask! 🌟

Just a quick note on TDD test building. As I mentioned in an earlier comment, we want to start with simpler inputs and get more complex as we go. Here, your tests are all good but not in the right order. If you were building the function step-by-step with TDD, you'd have them in the exact reverse order.

You don't need to change this but I wanted to highlight it to remind you of good TDD practices.

Liam310 · 2026-11-10T09:09:21Z

Hi @MattCarolus10 ! Sorry about the labels jumping around, I was using your PR to demonstrate a code review.

Good work overall, I've left some comments on a few things that could be improved. Take a look and let me know if you have any questions 🙂

…and consistency

…is passed

…function

…Lookup

… function

MattCarolus10 added 30 commits November 5, 2026 23:12

object prep

8531191

fix houseNumber error

037311d

fix author object iteration

13f83e1

add tests for parseQueryString function to validate query string parsing

3d72ba1

add function to double all numbers in an array

454659b

fix formatting in address.js comments for clarity

af702ab

fix recipe logging to display ingredients correctly

e6ed80a

fix comments in recipe.js to clarify logging functionality

e80fd6f

implement test for contains function to check empty object case

74c65b7

implement contains function to check if property exists in object

c61f58e

refactor contains.test.js to improve test structure and formatting

438293b

add test for contains function to verify behavior with missing property

4e217bc

add test to ensure contains function throws error for array input

b72528b

refactor contains function to validate input type before checking pro…

e8cf923

…perty existence

remove redundant comments and acceptance criteria from contains.test.js

8e89f1d

implement test for createLookup function to validate country-currency…

c39280d

… pairs

fix: update test to validate correct output for createLookup function

80034ab

implement createLookup function to map country-currency pairs

8c1878d

refactor: improve formatting of createLookup test and remove redundan…

62d2398

…t comments

refactor: simplify parseQueryString function by removing redundant ch…

b1fdc4d

…ecks and comments

refactor: clean up parseQueryString function by removing unnecessary …

05ccd8c

…comments and whitespace

fix: correct value assignment in parseQueryString and decode URI comp…

fe222e0

…onents

fix: decode URI components in parseQueryString function

89d8fda

fix: decode URI components in parseQueryString function

17adfe9

refactor: improve key and value decoding in parseQueryString function

4918e85

test: add test for handling empty query string

f573273

test: add test for handling values with both + and percent-encoding

05ee028

test: implement test for tally function with an empty array

15fe829

test: fix indentation in tally test cases

8eb4f2d

test: add assertion for counting unique items in tally function

967cdf6

Liam310 reviewed Nov 10, 2026

View reviewed changes

Liam310 added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Nov 10, 2026

MattCarolus10 added 17 commits November 10, 2026 16:55

fix: update comment for clarity on ingredient printing issue

2a43c8c

test: add example usage for calculateMode function

5082a4d

test: add case for contains function to throw error on array input

fcdb3b7

refactor: update test cases for contains function to improve clarity …

c167eac

…and consistency

test: add case for contains function to throw error on null input

4335eb3

test: add case for contains function to throw error when no argument …

417ac1b

…is passed

refactor: remove commented-out test cases for clarity

58d2351

test: uncomment test cases for array and object handling in contains …

5a54171

…function

test: add test case for creating a lookup with a single currency pair

f6f9b2a

test: add case to return an empty object for an empty array in create…

0a4fb24

…Lookup

fix: format whitespace in test cases for consistency

2343205

fix: correct expected value for single currency pair lookup test

0672819

refactor: rename variables for clarity in parseQueryString function

e9727d9

refactor: update tally tests for clarity and organization

745fa98

test: add case for counting occurrences of different letters in tally…

2470fea

… function

test: enable test for tally on an empty array

68b000d

refactor: remove commented-out tests for clarity in tally test file

3b5c96f

MattCarolus10 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 10, 2026

Navigation Menu

Repository navigation

Manchester | 26-ITP-Sep | Mathew Shaibu | Sprint 2 | Module data group sprint 2 - #1499

Manchester | 26-ITP-Sep | Mathew Shaibu | Sprint 2 | Module data group sprint 2#1499
MattCarolus10 wants to merge 60 commits into
CodeYourFuture:mainfrom
MattCarolus10:Module-data-group-sprint-2

MattCarolus10 commented Nov 6, 2026

Uh oh!

Liam310 Nov 10, 2026

Uh oh!

Liam310 Nov 10, 2026

Uh oh!

Liam310 Nov 10, 2026

Uh oh!

Liam310 Nov 10, 2026

Uh oh!

Liam310 Nov 10, 2026

Uh oh!

Liam310 Nov 10, 2026

Uh oh!

Liam310 commented Nov 10, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		const k = decodeURIComponent(key.split("+").join(" "));
		const v = decodeURIComponent(value.split("+").join(" "));

Conversation

MattCarolus10 commented Nov 6, 2026

Task code

Changelist

Uh oh!

Liam310 Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 commented Nov 10, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants