London | ITP-May-2025 | Jamal Laqdiem| Module-Data-Groups | Sprint 1 #557

jemaljemy · 2025-07-05T13:37:22Z

Learners, PR Template

Self checklist

I have committed my files one by one, on purpose, and for a reason
I have titled my PR with REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
I have tested my changes
My changes follow the style guide
My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

Sprint-1/fix/median.js

fearcyf · 2025-07-07T09:29:16Z

Sprint-1/implement/dedupe.test.js

Tests passed, well done. Could your approach be more succinct? Research test.each

done. thanks for reviewing.

fearcyf · 2025-07-07T09:33:47Z

Sprint-1/implement/max.test.js

Tests passed, well done

thanks for reviewing.

fearcyf · 2025-07-07T09:39:02Z

Sprint-1/implement/sum.test.js

Test passed, well done

thanks for reviewing.

fearcyf · 2025-07-07T09:46:43Z

Sprint-1/refactor/includes.js

Code works fine, just one consideration, do you really need lines 4-6. It's not an error just consider it.

you are right, even if with an if statement will get an early exit if the list is empty does not change much as the for..of loop handle this as well. thanks for reviewing.

fearcyf · 2025-07-07T10:00:46Z

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

sense checked the code - looks fine. This should work providing relevant file is available. Consider how you would code to catch and handle an error ( for example a file not being available). Just something to think about./research

Thanks for reviewing, I will do.

fearcyf · 2025-07-07T10:03:49Z

A few points to consider, overall good work.

…rage.

… handling non-numeric values and empty inputs.

…tests.

fearcyf · 2025-07-08T13:45:52Z

Jamal, it says that your branch has conflicts to be resolved. Have a look at this.

…clean and more succinct.

jemaljemy · 2025-07-08T14:14:41Z

I'm trying from last night to get the issue of median.test.js file who is doing this, still trying to rebase.

illicitonion

This is generally looking good, but sometimes a bit more complicated than it could be. Please take a look at my comments and update your code or reply to them with thoughts!

Sprint-1/fix/median.js

illicitonion · 2025-07-17T16:18:57Z

Sprint-1/implement/max.js

@@ -1,4 +1,27 @@
 function findMax(elements) {
+  if (elements.length === 0) {


What are you guarding against with this if statement? If you removed it, what would break in your code?

thanks for reviewing, this if statement check in case of empty array will return -infinity as requested, it would return an incorrect and misleading result for an empty array.

What result would it return for an empty array, if you removed this special case? Read through the code line-by-line and try to work it out.

Thanks for reviewing, after examining the code again i can clearly notice that the if condition in the line 2 is redundant, as the for loop handle that case and return -infinity.

Sounds good! If you push a change fixing this, we can call this PR complete :)

Sprint-1/implement/sum.js

illicitonion · 2025-07-18T13:25:58Z

Please respond to all comments before requesting review again - it's ok to respond by changing code, or by replying to them with an answer.

jemaljemy · 2025-07-18T14:54:19Z

Thanks for taking time to review, from my side it look like I answered to your 4 comments, I may be missing something, could you please specify what is missing.
much appreciated.

illicitonion · 2025-07-21T10:23:42Z

Sorry, I can't see any answers - is it possible you have some comments pending, or some code changes you haven't pushed to your remote?

I see these comments without answers:

And this is the only code I see having changed since I left those comments, which only answers one of the comments:

jemaljemy · 2025-07-21T15:08:56Z

I had to submit the comments from file changed, as they were pending.

illicitonion

I'm glad we could work that out, thanks! Just one comment left to work through :)

illicitonion · 2025-07-22T16:55:17Z

Sprint-1/implement/max.js

@@ -1,4 +1,27 @@
 function findMax(elements) {
+  if (elements.length === 0) {


What result would it return for an empty array, if you removed this special case? Read through the code line-by-line and try to work it out.

jemaljemy added the Needs Review Participant to add when requesting review label Jul 5, 2025

fearcyf self-assigned this Jul 7, 2025

fearcyf added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Participant to add when requesting review labels Jul 7, 2025

fearcyf reviewed Jul 7, 2025

View reviewed changes

Sprint-1/fix/median.js Show resolved Hide resolved

fearcyf reviewed Jul 7, 2025

View reviewed changes

fearcyf added Reviewed Volunteer to add when completing a review and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jul 7, 2025

jemaljemy added the Needs Review Participant to add when requesting review label Jul 7, 2025

jemaljemy added 7 commits July 8, 2025 11:24

fix(median): Correctly calculate median for all test cases

301b0b4

feat: Implement dedupe function and add tests.

be12690

feat: Implement robust findMax function with comprehensive tests cove…

1038918

…rage.

Adds a sum function that correctly sums finite numbers in an array,…

95997b7

… handling non-numeric values and empty inputs.

Refactor the includes function to use a for .. of loop passing all …

849a7e8

…tests.

solve part 1 solution.

4a828b8

check status of the file

171eef5

jemaljemy added 5 commits July 8, 2025 15:10

median.test file

0d44e9f

deleted a file

dffbcb4

added the correct test file for calculateMedian function.

0600b63

Fix: replaced expect .toEqual using more concise .each to keep codes …

9b0d9d6

…clean and more succinct.

Fix: deleted the extra lines.

beb9e8e

jemaljemy force-pushed the sprint-1 branch from e564805 to beb9e8e Compare July 8, 2025 14:21

Merge branch 'CodeYourFuture:main' into sprint-1

1b085dc

illicitonion reviewed Jul 17, 2025

View reviewed changes

illicitonion removed the Needs Review Participant to add when requesting review label Jul 17, 2025

done

16245db

jemaljemy added Needs Review Participant to add when requesting review 📅 Sprint 1 Assigned during Sprint 1 of this module labels Jul 18, 2025

illicitonion removed the Needs Review Participant to add when requesting review label Jul 18, 2025

jemaljemy added the Needs Review Participant to add when requesting review label Jul 18, 2025

illicitonion reviewed Jul 22, 2025

View reviewed changes

illicitonion removed the Needs Review Participant to add when requesting review label Jul 22, 2025

done

0b91582

jemaljemy added the Needs Review Participant to add when requesting review label Jul 23, 2025

illicitonion added Complete Volunteer to add when work is complete and review comments have been addressed and removed Needs Review Participant to add when requesting review Reviewed Volunteer to add when completing a review labels Jul 23, 2025

		@@ -1,4 +1,27 @@
		function findMax(elements) {
		if (elements.length === 0) {

Uh oh!

London | ITP-May-2025 | Jamal Laqdiem| Module-Data-Groups | Sprint 1 #557

Are you sure you want to change the base?

London | ITP-May-2025 | Jamal Laqdiem| Module-Data-Groups | Sprint 1 #557

Uh oh!

Conversation

jemaljemy commented Jul 5, 2025

Learners, PR Template

Changelist

Questions

Uh oh!

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!

fearcyf commented Jul 7, 2025

Uh oh!

fearcyf commented Jul 8, 2025

Uh oh!

jemaljemy commented Jul 8, 2025

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

illicitonion commented Jul 18, 2025

Uh oh!

jemaljemy commented Jul 18, 2025

Uh oh!

illicitonion commented Jul 21, 2025

Uh oh!

jemaljemy commented Jul 21, 2025

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!