London | ITP-SEP-25 | Samuel Tarawally | Sprint 2 | Coursework/sprint 2 #894
Conversation
Tarawally
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you revert the changes made in the "Sprint-3" folder to keep this branch clean?
Sprint-2/implement/contains.test.js
Outdated
|
|
||
| // Handles invalid inputs gracefully | ||
| test("contains handles invalid parameters", () => { | ||
| expect(contains([], "a")).toBe(false); |
There was a problem hiding this comment.
Arrays are objects in JavaScript, and they do have property names -- just not the same ones as objects.
Which keys do arrays have, and how does that affect how reliable your test is?
When testing whether the function handles arrays properly, try using a key that an array might
realistically contain. Otherwise, you might get a passing test even if the function isn't checking for arrays at all.
There was a problem hiding this comment.
I see that testing for "a" passed simply because that key wasn't there, not because the code was rejecting arrays. Since arrays have a "length" property. I'll update the test to check for that.
| for (const pair of arrayOfPairs) { | ||
| const key = pair[0]; | ||
| const value = pair[1]; |
There was a problem hiding this comment.
Could also consider using array destructuring to simplify this 3 lines of code.
There was a problem hiding this comment.
I'll remove the need for the temporary key and value' variables inside the loop. I'll refactor it to:
for (const [key, value] of arrayOfPairs) {
lookupObject[key] = value;
}
Sprint-2/implement/querystring.js
Outdated
| const firstEqualsIndex = pair.indexOf("="); | ||
|
|
||
| const key = pair.slice(0, firstEqualsIndex); | ||
| const value = pair.slice(firstEqualsIndex + 1); |
There was a problem hiding this comment.
What if the query string is "key1&key2" (i.e., without =)?
Sprint-2/implement/querystring.js
Outdated
| const key = pair.slice(0, firstEqualsIndex); | ||
| const value = pair.slice(firstEqualsIndex + 1); | ||
|
|
||
| queryParams[key] = value; |
There was a problem hiding this comment.
In real query string, both key and value are percent-encoded or URL encoded.
For example,
tags%5B%5D=hello%20world-> key istags[], value ishello world
Can your function handle URL-encoded query string?
Suggestion: Look up "How to decode a URL-encoded string in JavaScript".
There was a problem hiding this comment.
No, the current function cannot handle them. I think it would leave characters like %20 encoded.
I'll patch this by wrapping the keys and values in decodeURIComponent, which ensures they are correctly converted back to plain text as a work around.
Sprint-2/implement/tally.js
Outdated
| throw new Error("Input must be an array"); | ||
| } | ||
|
|
||
| const countObject = {}; |
There was a problem hiding this comment.
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.
No, it tries to perform arithmetic on a function, resulting in a broken output.
I'll change this to Object.create(null).
cjyuan
added
Reviewed
|
Yep, sure. I thought I’d checked out another branch. |
Tarawally
force-pushed
the
sprint-2
branch
3 times, most recently
from
627293b to
587720a
Compare
|
I'll be opening a new pr because i'm having issues with |
2 participants
Learners, PR Template
Self checklist
Changelist
address.js,author.js, andrecipe.jscontains,lookup,tally, andquerystringfunctions with testsinvert.js