ZA-ITP-May-2025 | Christian Mayamba | Module-Data-Groups | Week 1 #573
Conversation
|
Well done, Do some further research on the test.each approach in making code more concise. Also use const when variable does not need reassignment. |
fearcyf
added
Reviewed
There was a problem hiding this comment.
There are some significant things for you to look at around making sure you're solving the most general version of a problem, not just passing the tests which happen to be included. Please give this some thought and try those exercises again, working with volunteers if you need help!
| if ( | ||
| list.every( | ||
| (item) => typeof item === "string" || item === null || item === undefined | ||
| ) | ||
| ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This code looks like it wouldn't handle extra test-cases we could write, e.g. what would happen if the list contained only boolean values (true/false)?
We want to write code which is general - that is, it fixes the core problem statement, rather than a particular set of tests - please try to rewrite this so that it's more general.
| } | ||
|
|
||
| if (numbersArray.length % 2 === 0) { | ||
| const tempMedian = |
There was a problem hiding this comment.
tempMedian isn't a very clear name for this variable - can you think of a more clear name?
| const median1 = tempMedian / 2; | ||
| return median1; |
There was a problem hiding this comment.
Is there a reason you're calling this median1 rather than just median?
(And in fact, maybe you don't need a variable at all here - what value do you think the variable provides over just writing return tempMedian / 2;?)
| return median1; | ||
| } else { | ||
| const middleIndex = Math.floor(numbersArray.length / 2); | ||
| const median = numbersArray.splice(middleIndex, 1)[0]; |
There was a problem hiding this comment.
This branch uses splice whereas the other branch doesn't - why are you getting elements differently in the two branches? What does splice do which is maybe useful or unhelpful?
| if (array.length === 0) { | ||
| return []; | ||
| } | ||
|
|
||
| const duplicates = array.filter( | ||
| (item, index) => array.indexOf(item) !== index | ||
| ); | ||
|
|
||
| const set2 = new Set(duplicates); | ||
| return array.filter((item) => !set2.has(item)); |
There was a problem hiding this comment.
This feels like quite a complicated way of deduplicating an array. I'm also not quite sure it's behaving as the comments in the test file suggest. Can you give this another try? First focus on making sure you understand the requirements, then see if you can make it simpler.
| // Then it should remove the duplicate values, preserving the first occurence of each element | ||
| test("array with strings and numbers, it should remove duplicates", () => { | ||
| expect(dedupe([10, "hi", "hey", 20, "hey", 30])).toEqual([10, "hi", 20, 30]); |
There was a problem hiding this comment.
I don't think this test does the same as the comment describes. The comment says "preserves the first occurence", but your test doesn't keep the first "hey".
| return "-Infinity"; | ||
| } | ||
|
|
||
| if (elements.every((item) => typeof item === "string")) { |
There was a problem hiding this comment.
Same question as before - is "I can't find the max of an array of only strings" really the general statement here?
| } | ||
|
|
||
| if (elements.every((item) => typeof item === "string")) { | ||
| return "undefined"; |
There was a problem hiding this comment.
Returning the string "undefined" seems like an odd choice here. Can you think of a more appropriate value to return?
| return 0; | ||
| } | ||
|
|
||
| if (elements.every((item) => typeof item === "string")) { |
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.