-
-
Notifications
You must be signed in to change notification settings - Fork 160
London | 25-ITP-May | Houssam Lahlah | Sprint 2 | Sprint-2 #786
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
base: main
Are you sure you want to change the base?
Conversation
… keys, duplicates and leading '?'
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.
Code is very solid! I only have a few suggestions.
Sprint-2/implement/contains.js
Outdated
} | ||
|
||
// Use Object.prototype.hasOwnProperty to check if property exists | ||
return Object.prototype.hasOwnProperty.call(obj, prop); |
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.
There is another (more compact) function in Object
we can use. Can you find out how we can rewrite line 8 with less code.
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.
Hi cjyuan,
Thank you for the feedback!
I updated the code to use Object.hasOwn(obj, prop) instead of Object.prototype.hasOwnProperty.call(obj, prop).
It’s definitely more compact and readable.
Sprint-2/implement/tally.js
Outdated
const result = {}; | ||
for (const item of items) { | ||
if (Object.prototype.hasOwnProperty.call(result, item)) { |
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.
There is a way to create an empty object with no inherited properties.
If result
is such an object, then we can simplify line 8 as if (result[item]) {
.
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.
Hi cjyuan,
Thank you for pointing this out!
I changed result = {} to result = Object.create(null), so the object has no inherited properties. This allowed me to simplify the condition to if (result[item]) { as suggested.
Sprint-2/stretch/count-words.js
Outdated
const counts = {}; | ||
|
||
// 1. Remove punctuation | ||
const noPunctuation = str.replace(/[.,!?]/g, ""); | ||
|
||
// 2. Ignore case | ||
const words = noPunctuation.toLowerCase().split(/\s+/); | ||
|
||
for (const word of words) { | ||
if (!word) continue; // skip empty strings | ||
counts[word] = (counts[word] || 0) + 1; | ||
} |
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.
Can you check if this function returns what you expect in the following function call?
countWords("constructor constructor");
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.
Hi cjyuan,
Thanks for your guidence.
I tested with "constructor constructor" and saw the issue. I updated counts to use Object.create(null) instead of {}, so now the function correctly returns { constructor: 2 }.
Sprint-2/stretch/till.js
Outdated
total += coin * quantity; | ||
// coin is a string like "1p", "20p" | ||
// We need to strip the "p" and convert it into a number of pennies | ||
const coinValue = parseInt(coin.replace("p", ""), 10); |
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.
We could also rewrite line 13 as:
const coinValue = parseInt(coin);
Can you figure out why?
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.
Hi cjyuan,
Thanks! I see why — parseInt("20p", 10) returns 20 since it stops parsing at the "p".
That makes .replace("p", "") unnecessary, so I simplified the code.
Have you try asking AI?
The spec was not clear how
It seems you had figured out the answer.
You can also try AI for questions like this. :) |
Everything looks great. Well done! |
Self checklist
Changelist
1. Debug
2. Implement
3. Interpret
4. Stretch
Questions
Challenge Notes
1. Iterating over objects
2. contains(obj, prop) behavior
3. invert({ a: 1, b: 2 }) bug
4. totalTill returning NaN
const coinValue = parseInt(coin, 10);
total += coinValue * quantity;