NOT-A-STUDENT-PR: Update Solution and tests for Sprint 1 'fix median' task #390
Conversation
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
Thanks! This generally looks good, but I have one question about introducing throw
Sprint-1/fix/median.js
Outdated
| if (list.length % 2 === 0) { // use the remainder operator (%) to check if the array length is even | ||
| return (list[middleIndex - 1] + list[middleIndex]) / 2; | ||
| if (!onlyNumberList.length) { | ||
| throw new Error('You provided a list without numbers.'); |
There was a problem hiding this comment.
We haven't taught our trainees about throwing at this point (or in fact really at all in the course, which we should) - given the solutions branch is meant to be "What we'd expect our trainees to produce with their current knowledge" rather than "Ideal code", I'd be tempted to return null here?
Alternatively, we could introduce throw and try/catch at the start of this sprint, which would probably make sense? WDYT?
There was a problem hiding this comment.
@illicitonion Hey, Daniel. I totally agree with you. After I created this PR I was thinking that this is probably too complicated and as you rightly say - you probably didn't teach throw and it is not correct to add it to the solution.
I would suggest to have two solutions: one just a good solution with return null, and one, as an example, with throw as an illustration how this method can be improved. I will push update so that you can take a look.
There was a problem hiding this comment.
Hey @illicitonion please take a look at updated version:
- removed throw error
- return null in case not an array or array with non-numeric values provided
I also keep a commit with both versions of this function (returning null and trowing an error) and related unit tests just in case we would like to add throw to learning materials and this example will be needed: a6fd706
Once this PR is approved and merged, I will create the same tests for main branch.
| expect(list).toEqual([1, 2, 3]); | ||
| }); | ||
|
|
||
| [ 'not an array', 123, null, undefined, {}, [], ["apple", null, undefined] ].forEach(val => |
There was a problem hiding this comment.
We should add these tests to the main branch too, right?
There was a problem hiding this comment.
Absolutely, the main should also be updated. I will first update the tests as to my comment above - not to expect throw and after we merge these changes into solutions, I will create similar change to main.
|
Hi @illicitonion Do I need to get any more approvals or you can merge it? |
|
Merged! Thank you! If you can back-port the new/updated tests to |
…ure#390) * Update solution and tests for Sprint 1 'fix' exercise with median * Add tests to check for errors and mixed arrays (advanced implementation of median) * Create two vesions of calculate median function: with throw error and with return null * Remove calculate median with throwing error and related tests
|
Created similar change for |
2 participants
This PR suggests a few improvements in the proposed solution for
calculateMedian()method.The solution example now will have two advanced features:
It also adds a very important step in calculating a median - sorts the array.
The PR also updates unit tests. If we don't think that student's implementation should handle non-numeric arrays or throw errors, I can remove tests related to these cases.