LONDON-JAN-25 | ANDREI FILIPPOV | Module-Data groups| WEEK 1 #1
Conversation
📝 WalkthroughWalkthroughThis PR implements and refines utility functions with comprehensive test coverage, refactors an existing loop pattern, and adds a stretch challenge solution. Changes include fixing median calculation to avoid input mutation, implementing dedupe with validation, adding numeric filtering to max/sum functions, replacing test placeholders with concrete test suites, simplifying iteration patterns, and creating a new file-reading frequency accumulator. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Sprint-1/implement/dedupe.js`:
- Line 10: The call dedupe(["a", "a", "a", "b", "b", "c"]) is unused and appears
to be leftover test/debug code; either remove this standalone invocation or
assign its return value to a variable (e.g., const result = dedupe(...)) or
otherwise use it so the result isn't discarded; update the code around the
dedupe function accordingly to avoid side-effect-free calls.
In `@Sprint-1/implement/sum.js`:
- Around line 2-10: The accumulator is assigned to an undeclared identifier
"sum", leaking a global and potentially shadowing the module/function named sum;
make the accumulator local and avoid the name collision by declaring a local
variable (e.g., let total = 0) and use that inside the forEach (or replace the
whole loop with filteredByNumber.reduce((acc, n) => acc + n, 0)); update the
return to return the local variable (total or the reduce result) instead of the
undeclared sum.
In `@Sprint-1/implement/sum.test.js`:
- Around line 35-37: The test asserts an exact equality for the sum of
floating-point numbers which can fail due to JS floating-point precision; update
the test in sum.test.js that calls sum([1.1, 2.2, 3.3]) to use Jest's
toBeCloseTo matcher instead of toEqual (e.g., replace the expect(...)
.toEqual(6.6) with expect(...) .toBeCloseTo(6.6, <precision>) so the sum
function's result is compared with a tolerance; keep the same test name and call
site for sum to make the change minimal.
In `@Sprint-1/stretch/aoc-2018-day1/solution.js`:
- Around line 7-9: The code creates implicit globals by assigning to
listOfValues and using an unscoped for (change of ...) loop; update the function
that contains this snippet to declare these as local variables (e.g., use const
or let for listOfValues and for change) so they are block-scoped and safe under
strict mode—specifically change the assignment to declare listOfValues with
const/let and change the loop to for (const change of listOfValues) or for (let
change of listOfValues) to avoid global leaks.
🧹 Nitpick comments (1)
Sprint-1/implement/sum.test.js (1)
44-49: Interesting design choice - returning a string for invalid inputI see the function returns
"Enter valid numbers"when given an array with only non-numeric values. This mixes return types (sometimes a number, sometimes a string). What are the pros and cons of this approach versus, say, throwing an error or returningNaNor0?This isn't necessarily wrong - it matches your acceptance criteria - but it's worth thinking about how calling code would need to handle this. Would they check
typeof result === 'string'before using it?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignoreSprint-1/fix/median.jsSprint-1/implement/dedupe.jsSprint-1/implement/dedupe.test.jsSprint-1/implement/max.jsSprint-1/implement/max.test.jsSprint-1/implement/sum.jsSprint-1/implement/sum.test.jsSprint-1/refactor/includes.jsSprint-1/stretch/aoc-2018-day1/solution.js
🧰 Additional context used
🧬 Code graph analysis (3)
Sprint-1/implement/sum.js (2)
Sprint-1/implement/max.js (1)
filteredByNumber(2-2)Sprint-1/implement/sum.test.js (1)
sum(9-9)
Sprint-1/implement/max.js (1)
Sprint-1/implement/sum.js (1)
filteredByNumber(2-2)
Sprint-1/implement/dedupe.js (1)
Sprint-1/implement/dedupe.test.js (1)
dedupe(1-1)
🔇 Additional comments (8)
.gitignore (1)
5-5: Nice addition to keep prep materials out of version control!Adding
prepto your .gitignore is a clean way to keep those files local. I noticed you're using the simple patternprep, which will match any file or directory named "prep" throughout your repository tree (similar to hownode_modulesworks on line 1).Just something to think about: if you ever wanted to be more specific about where the prep folder lives, you have options:
/prepwould match only at the repository root**/prepwould explicitly match in any directory (thoughprepalready does this by default)Your current approach is totally fine and follows the same pattern as the rest of your .gitignore file!
Sprint-1/fix/median.js (1)
5-14: Looks good—non‑mutating median logic is clear.
I like the straightforward odd/even handling here; it keeps the input intact.Sprint-1/implement/max.js (1)
1-7: Nice—numeric filtering behavior is consistent with the tests.
This keeps non‑numeric inputs from skewing results and aligns with the suite expectations.Sprint-1/refactor/includes.js (1)
3-9: Clean refactor—loop reads more clearly now.
The early‑return behavior stays intact while improving readability.Sprint-1/implement/max.test.js (1)
19-60: Solid test coverage across edge cases.
The scenarios map well to the expected behavior, including non‑numeric handling.Sprint-1/implement/dedupe.js (1)
1-9: Nice implementation preserving insertion order!The logic here correctly handles deduplication while maintaining the original order of first occurrences. I notice you've used
aas the parameter name - what do you think about using something more descriptive likearroritems? When you come back to this code in a few months, which would be clearer at a glance?Sprint-1/implement/dedupe.test.js (1)
15-36: Solid test coverage!Great job implementing concrete tests that align with the acceptance criteria. You've covered the key scenarios: empty array, no duplicates, and multiple duplicate patterns.
One small observation - I notice "occurence" appears in lines 30-31. How many 'r's does "occurrence" have? 😉 It's a commonly misspelled word, so no worries - just something to tidy up if you get a chance.
Sprint-1/implement/sum.test.js (1)
12-43: Well-structured test suite!Nice work covering the key scenarios: empty arrays, single elements, negatives, and mixed content. The test descriptions clearly match the acceptance criteria comments, which makes it easy to trace requirements to tests.
One tiny thing: line 35's test description has a leading space (
" Given an array..."). Does that affect anything functionally? No - but consistency is a good habit!
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| }); | ||
| return listOfUniqueValues; | ||
| } | ||
| dedupe(["a", "a", "a", "b", "b", "c"]); |
There was a problem hiding this comment.
Unused function call - is this intentional?
I see there's a call to dedupe(["a", "a", "a", "b", "b", "c"]) here, but the result isn't being captured or used anywhere. This looks like it might be leftover debug/test code. What happens to the return value if nothing stores it? Would you want to remove this, or was there a specific reason to keep it?
🧹 Suggested removal
-dedupe(["a", "a", "a", "b", "b", "c"]);
module.exports = dedupe;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dedupe(["a", "a", "a", "b", "b", "c"]); | |
| module.exports = dedupe; |
🤖 Prompt for AI Agents
In `@Sprint-1/implement/dedupe.js` at line 10, The call dedupe(["a", "a", "a",
"b", "b", "c"]) is unused and appears to be leftover test/debug code; either
remove this standalone invocation or assign its return value to a variable
(e.g., const result = dedupe(...)) or otherwise use it so the result isn't
discarded; update the code around the dedupe function accordingly to avoid
side-effect-free calls.
| let filteredByNumber = elements.filter((num) => typeof num === "number"); | ||
| if (filteredByNumber.length == 0) { | ||
| return elements.length == 0 ? 0 : "Enter valid numbers"; | ||
| } else { | ||
| sum = 0 | ||
| filteredByNumber.forEach((item) => { | ||
| sum += item; | ||
| }); | ||
| return sum; |
There was a problem hiding this comment.
Declare the accumulator—implicit global here is risky.
Line 6 assigns to sum without let/const, which leaks a global and can collide with other code. How could you make the accumulator local and avoid shadowing the function name?
💡 Proposed fix
- sum = 0
+ let total = 0;
filteredByNumber.forEach((item) => {
- sum += item;
+ total += item;
});
- return sum;
+ return total;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let filteredByNumber = elements.filter((num) => typeof num === "number"); | |
| if (filteredByNumber.length == 0) { | |
| return elements.length == 0 ? 0 : "Enter valid numbers"; | |
| } else { | |
| sum = 0 | |
| filteredByNumber.forEach((item) => { | |
| sum += item; | |
| }); | |
| return sum; | |
| let filteredByNumber = elements.filter((num) => typeof num === "number"); | |
| if (filteredByNumber.length == 0) { | |
| return elements.length == 0 ? 0 : "Enter valid numbers"; | |
| } else { | |
| let total = 0; | |
| filteredByNumber.forEach((item) => { | |
| total += item; | |
| }); | |
| return total; |
🤖 Prompt for AI Agents
In `@Sprint-1/implement/sum.js` around lines 2 - 10, The accumulator is assigned
to an undeclared identifier "sum", leaking a global and potentially shadowing
the module/function named sum; make the accumulator local and avoid the name
collision by declaring a local variable (e.g., let total = 0) and use that
inside the forEach (or replace the whole loop with filteredByNumber.reduce((acc,
n) => acc + n, 0)); update the return to return the local variable (total or the
reduce result) instead of the undeclared sum.
| test(" Given an array with decimal/float numbers, return the correct total sum", () => { | ||
| expect(sum([1.1, 2.2, 3.3])).toEqual(6.6); | ||
| }); |
There was a problem hiding this comment.
Floating point precision gotcha ahead!
Have you tried running this test yet? Here's something interesting to try in your browser console or Node REPL:
console.log(1.1 + 2.2 + 3.3);What do you get? In JavaScript (and most languages), floating point arithmetic can produce surprising results like 6.6000000000000005 instead of 6.6. How might you adjust this test to handle that reality? Jest has a matcher called toBeCloseTo() that's designed for exactly this situation!
💡 Suggested fix using toBeCloseTo
test(" Given an array with decimal/float numbers, return the correct total sum", () => {
- expect(sum([1.1, 2.2, 3.3])).toEqual(6.6);
+ expect(sum([1.1, 2.2, 3.3])).toBeCloseTo(6.6);
});🤖 Prompt for AI Agents
In `@Sprint-1/implement/sum.test.js` around lines 35 - 37, The test asserts an
exact equality for the sum of floating-point numbers which can fail due to JS
floating-point precision; update the test in sum.test.js that calls sum([1.1,
2.2, 3.3]) to use Jest's toBeCloseTo matcher instead of toEqual (e.g., replace
the expect(...) .toEqual(6.6) with expect(...) .toBeCloseTo(6.6, <precision>) so
the sum function's result is compared with a tolerance; keep the same test name
and call site for sum to make the change minimal.
| listOfValues = values.split("\n"); | ||
| for (change of listOfValues) { | ||
| frequency += +change; |
There was a problem hiding this comment.
Avoid implicit globals for listOfValues and change.
Right now Line 7 and Line 8 create globals; if this ever runs under strict mode (or is imported), it will throw. How could you keep these scoped to the function?
💡 Proposed fix
- listOfValues = values.split("\n");
- for (change of listOfValues) {
+ const listOfValues = values.split("\n");
+ for (const change of listOfValues) {
frequency += +change;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| listOfValues = values.split("\n"); | |
| for (change of listOfValues) { | |
| frequency += +change; | |
| const listOfValues = values.split("\n"); | |
| for (const change of listOfValues) { | |
| frequency += +change; |
🤖 Prompt for AI Agents
In `@Sprint-1/stretch/aoc-2018-day1/solution.js` around lines 7 - 9, The code
creates implicit globals by assigning to listOfValues and using an unscoped for
(change of ...) loop; update the function that contains this snippet to declare
these as local variables (e.g., use const or let for listOfValues and for
change) so they are block-scoped and safe under strict mode—specifically change
the assignment to declare listOfValues with const/let and change the loop to for
(const change of listOfValues) or for (let change of listOfValues) to avoid
global leaks.
1 participant
Learners, PR Template
Self checklist
Changelist
Completed sprint 1 exercises
Questions
no questions I think
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.