Manchester| ITP-25-SEP | Fithi Teklom| Sprint 2 | Coursework #898
Conversation
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (Sprint-2) doesn't match expected format (example: 'Sprint 2', without quotes) 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. |
Fithi-Teklom
changed the title
Fithi-Teklom
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you improve the indentation of the code?
After installing the VSCode "prettier" extension, you can use VSCode "Format document" function to auto indent JS code.
Sprint-2/debug/recipe.js
Outdated
| ${recipe.ingredients[0]}: | ||
| ${recipe.ingredients[1]}: | ||
| ${recipe.ingredients[2]}: | ||
| ${recipe.ingredients[3]}: |
There was a problem hiding this comment.
Can you use an approach that works for any number of ingredients?
| // Then it should return false or throw an error | ||
|
|
||
| test("contains array-like object,invalid object and null returns false",() => { | ||
| 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.
Sprint-2/implement/querystring.js
Outdated
| if (idx === -1) { | ||
| // No '=' found, treat entire pair as key with empty value | ||
| queryParams[pair] = ""; | ||
| } else { | ||
| const key = pair.substring(0, idx); | ||
| const value = pair.substring(idx + 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".
Sprint-2/implement/tally.js
Outdated
| throw new Error("Input must be an array"); | ||
| } | ||
|
|
||
| const result = {}; |
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.
| "abort-controller": "^3.0.0", | ||
| "array-buffer-byte-length": "^1.0.2", | ||
| "arraybuffer.prototype.slice": "^1.0.4", | ||
| "async-function": "^1.0.0", | ||
| "available-typed-arrays": "^1.0.7", | ||
| "base64-js": "^1.5.1", | ||
| "buffer": "^6.0.3", | ||
| "call-bind": "^1.0.8", | ||
| "call-bind-apply-helpers": "^1.0.2", | ||
| "call-bound": "^1.0.4", | ||
| "data-view-buffer": "^1.0.2", | ||
| "data-view-byte-length": "^1.0.2", |
There was a problem hiding this comment.
Do you need all these dependencies (specified on lines 11-111) in your exercise?!
There was a problem hiding this comment.
I am not sure in which sprint of package.json you found this and where did they come from.
There was a problem hiding this comment.
You can view all changed files in this branch through the 'Changed files' tab on this PR, and find out which package.json it is.
cjyuan
added
Reviewed
Fithi-Teklom
added
Needs Review
|
Changes look good. |
cjyuan
added
Complete
3 participants
Self checklist