-
-
Notifications
You must be signed in to change notification settings - Fork 220
London | 25-ITP-September | Ammad Ur Rehman | Sprint 3 | [TECH ED] Complete sprint 2 exercises #836
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…t.js file to write tests.
cjyuan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. I just have a few suggestions.
| function tally(array) { | ||
| if (!Array.isArray(array)) throw new Error("array is not an array!"); | ||
|
|
||
| return array.reduce((accumulator, currentValue) => { | ||
| const value = accumulator[currentValue]; | ||
| accumulator[currentValue] = value ? value + 1 : 1; | ||
| return accumulator; | ||
| }, {}); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the following function call returns the value you expect?
tally(["toString", "toString"]);
Suggestion: Look up an approach to create an empty object with no inherited properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, I didn't know about this particular case! 😮
cjyuan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good! Well done.
| console.log( | ||
| `titie: ${recipe.title}, serves: ${ | ||
| recipe.serves | ||
| }, ingredients: ${recipe.ingredients.join(", ")}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requirement is "Each ingredient should be logged on a new line". :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Then why did you suggest Array.prototype.join()? My original implementation did exactly that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could join with new line character.
Learners, PR Template
Self checklist
Changelist
I completed all the exercises and skipped the stretch exercises
Questions
I’d like to point out the
parseQueryString()function, we should ensure it properly handles percent-encoded characters (e.g.,%3Dfor=and%2Bfor+).