Conversation
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
1 similar comment
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
Code Review for Learning objectives PRI'll now review the specific issues I found and post them as inline comments. |
There was a problem hiding this comment.
Inline Review Comments
Issue 1: Debug console.log in hasPairWithSum.js (line 31)
The console.log statement should be removed from production code:
console.log(hasPairWithSum([3, 2, 3, 4, 5], 9));
This appears to be debug code left in by accident.
Issue 2: Debug console.log in removeDuplicates.mjs (line 29)
The console.log statement should be removed from production code:
console.log(removeDuplicates([1, 2, 3, 4, 5, 1, 2, 3, 4, 5]));
This appears to be debug code left in by accident.
Summary Code ReviewWhat You DidYou optimized 4 core algorithms across both JavaScript and Python to reduce their time complexity:
What You Did Well ✅
Issues to Fix 🔧1. Remove debug console.log in hasPairWithSum.js (line 31)The file has a debug statement that shouldn't be in production code: console.log(hasPairWithSum([3, 2, 3, 4, 5], 9)); // ← Remove this lineWhy this is a problem: Debug/test statements in production code pollute logs and suggest unfinished work. Tests should validate this behavior, not console.log statements. Fix: Simply delete line 31. 2. Remove debug console.log in removeDuplicates.mjs (line 29)Same issue here: console.log(removeDuplicates([1, 2, 3, 4, 5, 1, 2, 3, 4, 5])); // ← Remove this lineWhy this is a problem: Same as above - this is test/debug code that should be removed. Fix: Simply delete line 29. SummaryYour algorithm optimizations are solid and well-documented! The complexity analysis is correct and the implementations are efficient. Just remove those two debug console.log statements and this PR will be ready to go. VERDICT: reviewed |
Inline Review CommentsSprint-1/JavaScript/removeDuplicates/removeDuplicates.mjsLine 32: The Issue: Debug/test code left in submitted work |
📋 Overall Review SummaryWhat You Did Well ✅
What Needs Improvement
|
Code Review SummaryGreat work on optimizing these algorithms! You've successfully identified inefficiencies and refactored them with significant complexity reductions. What you did well:✅ Excellent complexity analysis - Both files include detailed Big O complexity explanations for original and optimized versions Specific implementations:hasPairWithSum.js:
removeDuplicates.mjs:
Minor observations:The archived comments showing the original implementations are helpful for understanding the refactoring process. This is a good learning pattern! VERDICT: completed Your code meets all Sprint-1 requirements:
|
|
Do you mean having console.log is not good idea or is there any cases where we might have console.log in our code? |
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.