Repository navigation
London | 26-ITP-Sep | Bartosz Kawiak | Sprint 1 | Data Groups - #1478
bartoszkawiak wants to merge 10 commits into
Conversation
bartoszkawiak
added
📅 Sprint 1
|
|
||
| function calculateFrequency(input) { | ||
| let frequency = 0; | ||
| const nums = input.trim().split("\n").map(Number); |
There was a problem hiding this comment.
Is it necessary to call trim() on input?
There was a problem hiding this comment.
I would keep .trim() because it prevents an extra 0 from appearing when the file ends with a newline. However, for this particular Advent of Code task, removing .trim() wouldn't change the final sum because adding zero has no effect.
cjyuan
added
Reviewed
bartoszkawiak
added
Needs Review
There was a problem hiding this comment.
I forgot to raise this question in my previous review:
What should calculateMean([Infinity, 0, -Infinity]) or calculateMean([0, NaN]) return?
The same also applies to sum().
| if (elements.length === 0) { | ||
| return -Infinity; | ||
| } | ||
|
|
||
| if (elements.length === 1) { | ||
| return elements[0]; | ||
| } | ||
| const sortedList = elements.toSorted((a, b) => a - b); | ||
| return sortedList.at(-1); |
There was a problem hiding this comment.
Your code works, but there is a more efficient way to find the extreme value in an array without sorting it, as sorting can be a relatively costly operation.
This approach uses a simple loop and works with arrays of any size.
Since finding the maximum or minimum value in an array is a common programming task, could you research how this approach works and then implement it in your code?
There was a problem hiding this comment.
I believe it's sorted now. Thank you for the feedback CJ.
| } | ||
|
|
||
| for (const item of list) { | ||
| if (typeof item === "string" || item === null || list.includes(undefined)) { |
There was a problem hiding this comment.
Note: There are many types of non-number values. You could consider instead checking if the value is NOT a number, and optionally excludes NaN and +/- Infinity.
There was a problem hiding this comment.
it's fixed now CJ. Thank you
cjyuan
removed
the
Needs Review
bartoszkawiak
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good. Well done!
| if (elements.length === 0) { | ||
| return -Infinity; | ||
| } | ||
|
|
||
| let max = -Infinity; | ||
|
|
||
| for (const item of elements) { | ||
| if (item > max) { | ||
| max = item; | ||
| } | ||
| } | ||
| return max; |
There was a problem hiding this comment.
Note: The code on lines 14-21 also works when the array is empty.
cjyuan
added
Complete
Learners, PR Template
Self checklist
Task code
CYF-1071
Changelist
Completed the JavaScript exercises, added tests to check different cases, fixed errors, and refactored some functions to make the code cleaner and easier to read.