Repository navigation
Docs test: add an average helper - #9
colinhacks wants to merge 6 commits into
Conversation
|
Fixed the failing
|
There was a problem hiding this comment.
Caution
average([]) returns NaN, not 0. That breaks the requirement in #8, and the new test on test.js:8 fails, so node test.js (and CI) exits non-zero.
Reviewed changes
This review covers the new average helper and its tests.
average(numbers)inindex.js— sums the array withreduce, divides bylength, and adds the function tomodule.exports.- Tests in
test.js— check the mean of[1, 2, 3, 4]and the empty-array case.
claude-opus-5-5 (free via Pullfrog for OSS) | 𝕏
| module.exports = { add, subtract }; | ||
| function average(numbers) { | ||
| const total = numbers.reduce((sum, n) => sum + n, 0); | ||
| return total / numbers.length; |
There was a problem hiding this comment.
For an empty array this computes 0 / 0, which is NaN. #8 says an empty array should return 0. Running node test.js on this branch fails at test.js:8 with AssertionError: NaN !== 0.
| return total / numbers.length; | |
| if (numbers.length === 0) return 0; | |
| return total / numbers.length; |
|
@pullfrog make |
|
Some inputs still get through, because
|
|
@pullfrog three more changes, please. Track them with your todo list as you go:
|
|
I added
Decision for you: No todo-list tool was available in this run, so I tracked the three items by hand in the checklist above.
|
|
@pullfrog review |
There was a problem hiding this comment.
ℹ️ No critical issues — one validation gap and a nitpick worth a look.
Reviewed changes
Reviewed the full PR: two new array helpers, their tests, and new README API docs. node test.js passes locally.
average(numbers)— returns the arithmetic mean,0for an empty array, and throwsTypeErrorfor non-number-array input.median(numbers)— returns the middle value (or mean of the two middle values) from a sorted copy without mutating the input, with the same empty/validation behavior asaverage.- Tests — cover happy paths, empty arrays, single-element input, non-mutation, and a range of invalid inputs for both helpers.
- README — adds an
## APIsection that documentsadd,subtract,average, andmedian.
ℹ️ Nitpicks
- The PR title and description only mention
average, but the PR also addsmedianand documents all four helpers inREADME.md. Consider updating the description so it covers the full scope. test.js:29— theaverage([5])assertion sits apart from the otheraverageassertions (lines 7–14), after themedianblock and a blank line. Moving it up with the rest would keep each helper's tests together.
claude-opus-5-5 (free via Pullfrog for OSS) | 𝕏
|
|
||
| module.exports = { add, subtract }; | ||
| function average(numbers) { | ||
| if (!Array.isArray(numbers) || !numbers.every((n) => typeof n === "number")) { |
There was a problem hiding this comment.
Sparse arrays get past this check, because Array.prototype.every skips holes. average([1, , 3]) returns 1.333…: reduce also skips the hole, but the code still divides by length 3. median([1, , 3]) returns 3, because spreading turns the hole into undefined and sort moves it to the end. NaN also passes typeof n === "number", and then both helpers return NaN. The same check is copied at line 19, so whatever fix you choose belongs in both places, ideally in one shared validator.
Technical details
# Sparse arrays and NaN bypass input validation
## Affected sites
- index.js:10 — `average` validation uses `numbers.every(...)`, which skips holes; `typeof` accepts `NaN`
- index.js:19 — identical check in `median`
## Required outcome
- `average([1, , 3])` and `median([1, , 3])` throw `TypeError` (or otherwise don't silently return a wrong value)
- Decide whether `NaN` should be rejected; if so, both helpers throw `TypeError` for it
- Add matching `assert.throws` cases in test.js
## Suggested approach
- Extract one shared `assertNumberArray(numbers, name)` helper. Check every index with a plain `for` loop (or `Array.from(numbers).every(...)`) so holes come through as `undefined`, and use `Number.isFinite` / `!Number.isNaN` if `NaN` should be rejected.
This PR is test activity for the Pullfrog documentation screenshots. It will be closed without merging once the screenshots are captured.
Adds
average(numbers)toindex.jsand covers it intest.js. Refs #8.