NW | 25-ITP-Sep | TzeMing Ho | Sprint 1 | feature/destructuring #324
Conversation
TzeMingHo
added
📅 Sprint 1
illicitonion
added
Review in progress
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This is generally looking really good, but I left a few comments about how to make your code more readable.
illicitonion
added
Reviewed
TzeMingHo
added
Needs Review
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This is looking great! Just a few more small thoughts :)
| } | ||
|
|
||
| function teachersWithPets(hogwarts) { | ||
| return hogwarts.reduce((nameList, {firstName, lastName, pet, occupation}) => { |
There was a problem hiding this comment.
I'd similarly recommend a filter and map here :)
There was a problem hiding this comment.
Yes. I have updated the function teachersWithPets as well.
| const orderLineSpacing = [8, 20, 5] | ||
| let formattedOrderLine = '' | ||
| for (let index = 0; index < args.length; index++) { | ||
| formattedOrderLine += `${String(args[index]).padEnd(orderLineSpacing[index], " ")}` |
There was a problem hiding this comment.
Cute use of args!
One disadvantage of using ...args is it makes it less clear to someone trying to use your function what arguments it actually expects. I'd recommend adding a check at the start of the function which checks the right number of args was passed (and throws if not), and maybe also a documentation comment explaining what's expected and in which order.
There was a problem hiding this comment.
Would an error message when the length of arguments is not equal to 3 be good enough for users?
There was a problem hiding this comment.
yes, i think that'd be sufficient :) (don't forget the comment for the function!)
There was a problem hiding this comment.
I have added a comment for the function.
illicitonion
added
Reviewed
TzeMingHo
added
Needs Review
illicitonion
added
Complete
|
This looks great, well done! I'd recommend adding a JSDoc comment to the function which takes |
3 participants
Learners, PR Template
Self checklist
Changelist
I have completed the exercises with function destructuring the needed properties