Sheffield | 26-ITP-Jan | Mahmoud Shaabo | Sprint 2 | Module-Data-Groups by mahmoudshaabo1984 · Pull Request #1069 · CodeYourFuture/Module-Data-Groups

mahmoudshaabo1984 · 2026-03-22T15:52:26Z

Hello Reviewers / CodeYourFuture Team,

I have completed all mandatory and stretch exercises for Sprint 2. I made sure to thoroughly test my code, handle edge cases, and apply refactoring best practices.

Here is a summary of the progress:

Debug: Successfully fixed the bugs in address.js, author.js, and recipe.js by utilizing proper object iteration (Object.entries, Object.values) and correct property access.
Implement: Implemented the logic for contains, lookup, and tally. I also fixed the querystring function using indexOf("=") and slice() to correctly handle multiple equal signs securely.
Interpret: Refactored invert.js using bracket notation to dynamically assign keys and values. All analytical questions (a-e) have been clearly answered in the comments.
Stretch (Advanced): - count-words.js: Implemented Regex to remove punctuation, handled case insensitivity, and sorted the final results by frequency.
- mode.js: Successfully refactored the function into two smaller, single-responsibility functions (countFrequencies and findHighestFrequency) utilizing the Map object.
- till.js: Fixed the NaN bug by using parseInt() to accurately extract numerical values from strings.

Testing: Verified that all tests across all folders pass successfully using npx jest and manual node testing.

Personal Note for the Team:
Working with Map, Object.entries(), and data type parsing in this sprint greatly improved my understanding of how to safely manipulate complex objects in JavaScript. I have also ensured all my comments explain the "why" behind the code to be fully screen-reader compatible. I look forward to your feedback!

Best regards,
Mahmoud Shaabo

mahmoudshaabo1984 · 2026-03-22T15:56:49Z

Hi CJ,

I have just submitted my Pull Request for the Sprint 2 exercises. I really enjoyed the challenges in this sprint, especially learning how to refactor functions and using tools like Map, Object.entries(), and parseInt() to handle data safely.

All tests are passing, and I made sure my comments clearly explain the logic behind the code.

I am looking forward to your review and any feedback or suggestions you might have.

Thank you!

cjyuan

This exercise also expects the .test.js files in the "implement" folder to be updated. There are tests to be implemented.

cjyuan · 2026-03-25T17:28:07Z

+for (const ingredient of recipe.ingredients) {
+  console.log(ingredient);
+}


Your code works.

Here is an alternative worth exploring:
Since ingredient values are separated by '\n' in the output, we could also use
Array.prototype.join() to construct the equivalent string and then output the resulting string.

cjyuan · 2026-03-25T17:33:07Z

+    // Find the position of the FIRST "=" only
+    const firstEqualIndex = pair.indexOf("=");
+
+    // If there is no "=", the whole pair is the key with an empty value
+    if (firstEqualIndex === -1) {
+      queryParams[pair] = "";
+    } else {
+      // Everything before the first "=" is the key
+      const key = pair.slice(0, firstEqualIndex);
+      // Everything after the first "=" is the value (may contain more "=" signs)
+      const value = pair.slice(firstEqualIndex + 1);
+      queryParams[key] = value;
+    }


Does your function return the value you expect from the following function call?

parseQueryString("key1=value1&&key2=value2")

cjyuan · 2026-03-25T17:33:35Z

+  const counts = {};
+
+  // Loop through each item in the array
+  for (const item of items) {
+    // If this item already exists in counts, add 1 to it
+    // If it does not exist yet, start at 1
+    counts[item] = (counts[item] || 0) + 1;
+  }
+


Does the following function call returns the value you expect?

tally(["toString", "toString"]);

Suggestion: Look up an approach to create an empty object with no inherited properties.

cjyuan · 2026-03-25T17:35:59Z

+  const sortedCounts = {};
+  for (const [word, count] of sorted) {
+    sortedCounts[word] = count;
+  }


Can also explore using Object.fromEntries() to simplify the code.

cjyuan · 2026-03-25T17:38:01Z

  let freqs = new Map();

  for (let num of list) {


Could also consider replacing let by const to further improving the original code.

…ing edge case Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

mahmoudshaabo1984 · 2026-03-28T13:28:41Z

Hi CJ,

Thank you for the detailed review! I have addressed all your feedback. Here is a summary of the changes:

Mandatory fixes:

tally.js — Changed const counts = {}; to const counts = Object.create(null); to avoid inherited prototype properties like toString. Now tally(["toString", "toString"]) correctly returns { toString: 2 }.
querystring.js — Added a check to skip empty pairs (if (pair === "") continue;) inside the loop. Now parseQueryString("key1=value1&&key2=value2") correctly ignores the empty string between the double ampersands.
contains.test.js — Replaced test.todo with 4 real tests: empty object returns false, existing property returns true, non-existent property returns false, and array input returns false.
lookup.test.js — Replaced test.todo with 3 real tests: multiple pairs, single pair, and empty array.
tally.test.js — Replaced test.todo with 4 real tests: empty array, duplicate items, string input throws error, and items named "toString" are counted correctly.

Optional improvements (also applied):

count-words.js — Replaced the manual loop with Object.fromEntries(sorted) as you suggested.
mode.js — Changed let to const for freqs and the for...of loop variables, since they are not reassigned.

All 15 tests pass across implement/ and stretch/.

Looking forward to your feedback!

Best regards,
Mahmoud Shaabo

cjyuan

All other changes look good.

cjyuan · 2026-03-28T13:36:01Z

+test("contains returns false when passed an array instead of an object", () => {
+  expect(contains(["a", "b"], "a")).toBe(false);
+});


This test does not yet confirm that the function correctly returns false when the first argument is an array.
This is because contains(["a", "b"], "a") could also return false simply because "a" is not a key of the array.

Arrays are objects, with their indices acting as keys. A proper test should use a valid
key to ensure the function returns false specifically because the input is an array, not because the key is missing.

…tion

mahmoudshaabo1984 · 2026-03-28T13:50:09Z

Hi CJ,

Great catch — you are absolutely right. The previous test was passing for the wrong reason.

I have updated the test to use a valid array index as the key:

expect(contains(["a", "b"], "0")).toBe(false);

Since "0" is a valid key in the array, the only reason this returns false is because the function correctly rejects arrays. This properly confirms the intended behaviour.

All tests are passing. Thank you for the thorough review — I learned something important about writing meaningful tests!

Best regards,
Mahmoud

cjyuan · 2026-03-28T13:52:15Z

Changes look good. Well done.

illicitonion · 2026-05-04T20:21:18Z

Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it.

Complete all Sprint 2 exercises successfully

303455f

mahmoudshaabo1984 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 22, 2026

cjyuan reviewed Mar 25, 2026

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 25, 2026

This was referenced Mar 26, 2026

No code changes — task requires manual GitHub comment action mahmoudshaabo1984/ahmed-sami-web-course#1

Draft

Add Arabic explanation to README answering why Copilot couldn't post PR comments on external repos mahmoudshaabo1984/ahmed-sami-web-course#2

Open

mahmoudshaabo1984 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 28, 2026

Address CJ review: add tests, fix tally prototype issue, fix querystr…

2d34473

…ing edge case Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

cjyuan reviewed Mar 28, 2026

View reviewed changes

cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 28, 2026

Fix contains test: use valid array index to properly test array rejec…

51c11d3

…tion

mahmoudshaabo1984 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 28, 2026

mahmoudshaabo1984 mentioned this pull request Apr 8, 2026

Sheffield Jan 2026 | Data groups | Mahmoud Shaabo mahmoudshaabo1984/My-Coursework-Planner#33

Open

illicitonion closed this May 4, 2026

Uh oh!

Conversation

mahmoudshaabo1984 commented Mar 22, 2026

Uh oh!

mahmoudshaabo1984 commented Mar 22, 2026

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

mahmoudshaabo1984 commented Mar 28, 2026

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

mahmoudshaabo1984 commented Mar 28, 2026

Uh oh!

cjyuan commented Mar 28, 2026

Uh oh!

illicitonion commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants