-
-
Notifications
You must be signed in to change notification settings - Fork 211
Glasgow | 25-ITP-Sep | Abraham-Habte | Sprint 2 | Sprint2-exercise #820
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
…ion to pass the test
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.
-
Code is very solid.
-
Indentation is a bit off in some of the files. Can you improve the indentation on these files?
Sprint-2/debug/author.js
Outdated
| age: 40, | ||
| alive: true, | ||
| }; | ||
| console.log(author) |
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.
This works (for displaying the properties) to certain extent.
However, I think the objective of this exercise is to practice how to iterate through all the property values of an object. Can you figure out how to use a loop to accomplish 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.
got it
Sprint-2/implement/contains.js
Outdated
| return false; | ||
| } | ||
|
|
||
| return properityValue in object; |
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.
Suggestion
Consider the following two approaches for determining if an object contains a property:
let obj = {}, propertyName = "toString";
console.log( propertyName in obj ); // true
console.log( Object.hasOwn(obj, propertyName) ); // false
Which of these approaches suits your needs better?
For more info, you can look up JS "in" operator vs Object.hasOwn.
Sprint-2/implement/querystring.js
Outdated
| const index = pair.indexOf("="); | ||
| if (index === -1) { | ||
| queryParams[pair] = undefined; | ||
| } else { | ||
| const key = pair.slice(0, index); | ||
| const value = pair.slice(index + 1); | ||
| queryParams[key] = value; | ||
| } |
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.
Please note that in real querystring, both key and value are percent-encoded or URL encoded in the URL. For example, the string "5%" will be encoded as "5%25". So to get the actual value of "5%25" (whether it is a key or value in the querystring), you should call a function to decode it.
May I suggest looking up any of these terms, and "How to decode URL encoded string in JS"?
| function tally(input) { | ||
| if (!Array.isArray(input)) { | ||
| throw new Error("Input must be an array"); | ||
| } | ||
|
|
||
| const counts = {}; | ||
| for (let item of input) { | ||
| counts[item] = (counts[item] || 0) + 1; | ||
| } | ||
|
|
||
| return counts; | ||
| } |
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.
|
When you make changes based on review comments, please respond directly to each comment. Even a short acknowledgement is helpful. This makes the review process clearer, shows your reasoning, and ensures that nothing gets overlooked. Code review is a conversation, not just an edit pass. |
Sprint-2/implement/querystring.js
Outdated
|
|
||
| for (const pair of keyValuePairs) { | ||
| const index = pair.indexOf("="); | ||
| if (index === -1) { | ||
| queryParams[decodeURIComponent(pair)] = undefined | ||
| } else { | ||
| const key = pair.slice(0, index); | ||
| const value = pair.slice (index + 1); | ||
| queryParams[key] = value; | ||
| } | ||
| return queryParams; | ||
|
|
||
| return queryParams; | ||
| } | ||
| } |
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.
-
No all keys and values are properly URL-decoded.
-
The indentation is a bit off.
Sprint-2/implement/querystring.js
Outdated
| const queryParams = {}; | ||
| if (!queryString ) return queryParams; | ||
|
|
||
| const keyValuePairs = queryString.split("&"); | ||
|
|
||
| for (const pair of keyValuePairs) { | ||
| const index = pair.indexOf("="); | ||
| if (index === -1) { | ||
| queryParams[decodeURIComponent(pair)] = undefined | ||
| } else { | ||
| const key = decodeURIComponent (pair.slice(0, index)); | ||
| const value = decodeURIComponent (pair.slice (index + 1)); | ||
| queryParams[key] = value; | ||
| } | ||
|
|
||
| return queryParams; | ||
|
|
||
| } | ||
| } |
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 still not properly indented.
Why not use the VSCode built-in "Format document" feature (after installing the "prettier" VSCode extension) to auto format the code?
Learners, PR Template
Self checklist
Changelist
I fix the errors incounterd in the function as well as in the test and creat new function to pass the tests
Questions
no questions