London|25-ITP-September|Alexandru Pocovnicu|Sprint 1|Feature/destructuring#338
London|25-ITP-September|Alexandru Pocovnicu|Sprint 1|Feature/destructuring#338alexandru-pocovnicu wants to merge 7 commits intoCodeYourFuture:mainfrom alexandru-pocovnicu:feature/destructuring
Conversation
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. 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. |
4 similar comments
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. 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. |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. 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. |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. 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. |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. 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. |
alexandru-pocovnicu
added
the
Needs Review
jennetturkkan
added
Review in progress
jennetturkkan
left a comment
jennetturkkan
left a comment
There was a problem hiding this comment.
Hi @alexandru-pocovnicu! Good work here. I left couple of comments that are suggestions but please fix variable declarations, where you use let, you should use const unless you expect the value to change
| // Update the parameter to this function to make it work. | ||
| // Don't change anything else. | ||
| function introduceYourself(___________________________) { | ||
| function introduceYourself({name,age,favouriteFood }) { |
There was a problem hiding this comment.
It's correct but I think you Prettier is not working properly, the formatting is a bit off.
| }); | ||
|
|
||
| hogwarts.map(({ occupation, pet, firstName, lastName }) => { | ||
| if (occupation === "Teacher" && pet !== null) { |
There was a problem hiding this comment.
You could do it simpler by doing if (occupation === "Teacher" && pet) { because we know it's set to null when there is no pet. However, if pet could be empty string, number, undefined, false then if (pet) might give a wrong result.
You don't need to change anything, this is just an improvement you can keep in mind.
| let totalPence = quantity * unitPricePence; | ||
| let pence = totalPence % 100; | ||
| let paddedPence = String(pence).padStart(2, "0"); | ||
| let pounds = Math.floor(totalPence / 100); | ||
| let priceEachItem = `${pounds}.${paddedPence}`; |
| ); | ||
| }); | ||
| let sumAllPence = 0; | ||
| order.forEach(({ quantity, unitPricePence }) => { |
There was a problem hiding this comment.
Suggestion: you could write this with reduce and it would be much cleaner, this is a perfect use case for reduce.
jennetturkkan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Used array and object destructuring to get values, calculated and displayed a receipt