London | 26-ITP-May | Alex Jamshidi | Sprint 1 | Exercises #1225
Open
Alex-Jamshidi wants to merge 9 commits into
Open
London | 26-ITP-May | Alex Jamshidi | Sprint 1 | Exercises #1225Alex-Jamshidi wants to merge 9 commits into
Alex-Jamshidi wants to merge 9 commits into
Conversation
added 7 commits
May 12, 2026 16:25
Alex-Jamshidi
added
📅 Sprint 1
Member
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
LGTM, a few things to look at
| { input: ["hello", "a", "b", "c"], expected: ["hello", "a", "b", "c"] }, | ||
| { input: [1, 2, "hello", 4, "a", 6], expected: [1, 2, "hello", 4, "a", 6] }, | ||
| ].forEach(({ input, expected }) => | ||
| it(`returns the median for [${input}]`, () => expect(dedupe(input)).toEqual(expected)) |
Member
There was a problem hiding this comment.
This doesn't test one of the parts of the wording of the comment above, that it returns a copy of the original array. How could you test that the array returned is a copy, rather than the same array that was passed in?
Author
There was a problem hiding this comment.
I fixed this with this addition to the test:
it(`returns a non-identical array (copy) for [${input}]`, () =>
expect(dedupe(input)).not.toBe(input));
Where the input of the test is being passed to the dedupe function and the return of that is compared with the original input and checked 'not.toBe' the same
tests pass
| function findMax(list) { | ||
| const numbers = list.filter((n) => Number.isFinite(n)); | ||
| if (numbers.length == 0) { | ||
| return "-Infinity"; |
Member
There was a problem hiding this comment.
You always want to return a number here not a string. Infinity is a special numeric literal in JavaScript - it's a number like others (but a bit special).
Suggested change
| return "-Infinity"; | |
| return -Infinity; |
If you put the term in quotes, you're returning a string, not a number.
|
|
||
| it("empty array returns -Infinity", () => { | ||
| const list = []; | ||
| expect(findMax(list)).toEqual("-Infinity"); |
Member
There was a problem hiding this comment.
Suggested change
| expect(findMax(list)).toEqual("-Infinity"); | |
| expect(findMax(list)).toEqual(-Infinity); |
|
|
||
| it("array with only non-number values, return the least surprising value given how it behaves for all other inputs", () => { | ||
| const list = ["a", "b", "c"]; | ||
| expect(findMax(list)).toEqual("-Infinity"); |
Member
There was a problem hiding this comment.
Suggested change
| expect(findMax(list)).toEqual("-Infinity"); | |
| expect(findMax(list)).toEqual(-Infinity); |
illicitonion
added
Reviewed
Alex-Jamshidi
added
Needs Review
illicitonion
added
Complete
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Learners, PR Template
Self checklist
Changelist
Sprint-1 exercises completed