Manchester | 26-ITP-Jan | Mehroz Munir | Sprint 1 | Coursework exercises by MehrozMunir · Pull Request #1018 · CodeYourFuture/Module-Data-Groups

MehrozMunir · 2026-03-16T13:51:06Z

Learners, PR Template

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

Changelist

In this PR, I am submitting changes made in the exercise files.

ykamal

Good work so far. 🎉 Just a few things to clear out. Well done 👍 . Let me know when the changes are done.

ykamal · 2026-03-16T17:01:47Z

Sprint-1/fix/median.js

-  const middleIndex = Math.floor(list.length / 2);
-  const median = list.splice(middleIndex, 1)[0];
+  let median = null;
+  if (Array.isArray(list) && !list.every((item) => typeof item != "number")) {


Excellent check of valid types for each item in an array. 👍
There are a few things to improve here:

You're doing a double negative check here. One positive check would reduce the complexity.

Use the early return method, where if your data doesn't match a certain criterion, you just stop with either a return or an error. More: https://gomakethings.com/the-early-return-pattern-in-javascript/

Thank you so much, that was really helpful. I have implemented this now.

ykamal · 2026-03-16T17:05:58Z

Sprint-1/fix/median.js

-  const median = list.splice(middleIndex, 1)[0];
+  let median = null;
+  if (Array.isArray(list) && !list.every((item) => typeof item != "number")) {
+    numericList = list.filter((item) => typeof item === "number");


Hmm, something about the declaration of these variables numericList and sortedList doesn't seem right. Think about the scope, think about mutability. What keywords should you use here to ensure your scope is proper, to avoid implicit globals?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Grammar_and_types

Ahh yes, I now understand the scope better after reading this. Thanks.

ykamal · 2026-03-16T17:09:39Z

Sprint-1/fix/median.js

+      median = (medianArray[0] + medianArray[1]) / 2;
+    } else median = sortedList.splice(middleIndex, 1)[0];
+  }
  return median;


Nice, organised return statement. If the above code falls through, this will catch it.
But question ❓ Can a median be null? Does it have to be a number? Or is null fine?

Yes, it can't be null. I have added a check at the end for that. Let me know if it seems right. Thanks

ykamal · 2026-03-16T17:23:06Z

Sprint-1/fix/median.js

+    sortedList = numericList.toSorted((a, b) => a - b);
+    const middleIndex = Math.floor(sortedList.length / 2);
+    if (sortedList.length % 2 === 0) {
+      const medianArray = sortedList.splice(middleIndex - 1, 2);


Hmm. .splice() is a mutating operation. It changes the original array. The goal here is to access the value at a certain index of the array. In that regard, calculating the index would be a better solution for odd/even-length arrays. That will preserve the original array for later operations. Whereas right now, if you did more operations below, you'd be working with a mutated/changed array with fewer items in it.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice

Yeah, that's totally right. I have changed the code here to not use splice method and instead use the array index to access the values.

ykamal · 2026-03-16T17:25:01Z

Sprint-1/implement/dedupe.js

@@ -1 +1,12 @@
-function dedupe() {}
+function dedupe(arr) {
+  if (arr.length === 0) return arr;


Nice length check here 👍 But could the array just not be present, i.e. null/undefined?

Oh yes, I thought about these null/ undefined checks when working on these test files, but as this case was not described already, I thought we might not be supposed to do this.

But I think it is always a good practice to have those checks. I have added them now. Kindly check, please.

ykamal · 2026-03-16T17:27:24Z

Sprint-1/implement/dedupe.js

+  if (arr.length === 0) return arr;
+  else {
+    dedupeArray = [];
+    arr.forEach((element) => {


This is generally good logic. Well done 👍
However, if you want a much simpler approach, a better data structure would be a Set. You'd create a set from this array, then return an array of the set.
that'd do this for you.
https://www.geeksforgeeks.org/javascript/how-to-convert-set-to-array-in-javascript/

Thank you for introducing Set into my life. I am using them now.

Hehe, happy to do so. But ensure you follow the goals of the current chapter. There's always another advanced technique, but follow the goal/description of the task, even if it's painful to do so.

ykamal · 2026-03-16T17:35:34Z

Sprint-1/implement/max.js

@@ -1,4 +1,6 @@
-function findMax(elements) {
+function findMax(array) {
+  numbersArray = array.filter((value) => typeof value === "number");


Excellent type check. But also ensure to do null/undefined check

ykamal · 2026-03-16T17:37:27Z

Sprint-1/implement/sum.js

@@ -1,4 +1,10 @@
-function sum(elements) {
+function sum(array) {
+  numbersArray = array.filter((value) => typeof value === "number");


Same here, null/undefined check should be added

Also, look into .reduce() after this section is over.

Yes, I read about reduce, and it seems better to use that here.

ykamal · 2026-03-16T17:38:50Z

Sprint-1/refactor/includes.js

-    if (element === target) {
-      return true;
-    }
+  for (const element of list) {


Ah, you must use a for-loop, I see. Otherwise, .includes() would be fine. Poor thing 🤣

ykamal · 2026-03-16T17:40:36Z

Sprint-1/stretch/aoc-2018-day1/solution.js

@@ -0,0 +1,16 @@
+function getFinalFrequency() {
+  let finalFrequency = 0;
+  const fs = require("fs"); //getting file system to read file


require/imports should be at the top of the files. This is syntactically valid, but, practically discouraged.

Thank you for highlighting that. I will follow this from now on. Thanks.

MehrozMunir · 2026-03-17T14:53:55Z

Good work so far. 🎉 Just a few things to clear out. Well done 👍 . Let me know when the changes are done.

Thank you so much for your valuable feedback. I have committed the changes after your feedback. Please review my code now.

ykamal · 2026-03-17T23:26:00Z

Sprint-1/implement/max.js

-function findMax(elements) {
+function findMax(array) {
+  if (!Array.isArray(array)) throw new Error(array + " is not an array");
+  let numbersArray = array.filter((value) => typeof value === "number");


for variables that are not later reassigned, you can use const as it's recommended and is safer. A const won't be allowed to be reassigned so you can't accidentally redeclare the same variable or change it. (Unless it's an object, in that case you can still mutate its state using its methods).

Thanks I will do that from now on. Made this change in the code as well.

ykamal · 2026-03-17T23:27:14Z

Sprint-1/implement/sum.js

+function sum(array) {
+  if (!Array.isArray(array)) throw new Error(array + " is not an array");
+  let numbersArray = array.filter((value) => typeof value === "number");
+  let initialValue = 0;


It's good that you're getting in the habit of clean code, but for most things reduce(), you do not need to declare the initial value as a variable, just use it in the reduce().

ykamal · 2026-03-17T23:27:54Z

Sprint-1/stretch/aoc-2018-day1/solution.js

@@ -0,0 +1,17 @@
+const fs = require("fs"); //getting file system to read file


ykamal · 2026-03-17T23:29:40Z

Sprint-1/implement/dedupe.test.js

+  // Then it should return an empty array
+  it("given an empty array it should return an empty array", () => {
+    const array = [];
+    dedupeArray = dedupe(array);


I think this one slipped through the crack. Let's add a keyword

ykamal · 2026-03-17T23:31:48Z

Sprint-1/implement/dedupe.test.js

+  // When passed to the dedupe function
+  // Then it should thrown an error
+  [null, 930, "just a string", undefined, {}].forEach((val) =>
+    it("throw an error if the input is not an array", () =>


This is causing the test name to not be unique. Perhaps there is a better way to do this? Do the iteration inside the test? Or something better?

ykamal

Overall, well done 👍 There's a problem with unique test name in dedupe.test.js. Once that's sorted, we can close this one.
And median === null check will actually be reached so we can remove that check for now.

MehrozMunir · 2026-03-18T12:38:59Z

Overall, well done 👍 There's a problem with unique test name in dedupe.test.js. Once that's sorted, we can close this one. And median === null check will actually be reached so we can remove that check for now.

Thank you for the feedback. I have created separate test cases in the dupe.test.js file for these tests. I hope that is the right way to do this.

sprint-1 completed

3e3845b

MehrozMunir added 📅 Sprint 1 Assigned during Sprint 1 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Groups The name of the module. labels Mar 16, 2026

ykamal added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 16, 2026

ykamal suggested changes Mar 16, 2026

View reviewed changes

ykamal 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 Mar 16, 2026

fixed issues after the feedback

0448c28

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

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

ykamal reviewed Mar 17, 2026

View reviewed changes

ykamal suggested changes Mar 17, 2026

View reviewed changes

fixed issues after the feedback 2

64f84d9

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

		@@ -0,0 +1,17 @@
		const fs = require("fs"); //getting file system to read file

Uh oh!

Conversation

MehrozMunir commented Mar 16, 2026

Learners, PR Template

Changelist

Uh oh!

ykamal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MehrozMunir commented Mar 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykamal Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykamal left a comment

Choose a reason for hiding this comment

Uh oh!

MehrozMunir commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ykamal Mar 17, 2026 •

edited

Loading