London | 26-ITP-May | Damilola Odumosu | Sprint 2 | Coursework - #1352
London | 26-ITP-May | Damilola Odumosu | Sprint 2 | Coursework#1352d-odumosu wants to merge 20 commits into
Conversation
d-odumosu
added
Needs Review
cjyuan
added
Reviewed
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Could you also address the two comments related to querystring.js in the previous review?
| for (const value of author) { | ||
| console.log(value); | ||
| //An object is not directly iterable, if we want to log out the values we can use a for in loop | ||
| for (const value in author) { |
There was a problem hiding this comment.
The value in the loop is actually the key or property of the object. Naming it value is a bit misleading.
There was a problem hiding this comment.
changed the variable name to key, thank you
| if (!( | ||
| typeof object === "object" && | ||
| !Array.isArray(object) && | ||
| object !== null | ||
| )) { | ||
| throw new Error("Invalid data type"); | ||
| } |
There was a problem hiding this comment.
Double negatives makes the condition harder to read.
Suggestion: Look up "How De Morgan's Law works in programming?" and apply it to transform the condition.
| test.each([["hello"], [123], [true], [[]], [null]])( | ||
| "throws an error when input is %p", | ||
| (type) => { | ||
| expect(() => { | ||
| contains(type, "A"); | ||
| }).toThrow("Invalid data type"); | ||
| } | ||
| ); |
There was a problem hiding this comment.
The forth test data is also an array, do you mean to test undefined?
There was a problem hiding this comment.
I was initially testing an empty array but per your suggestions i have made a different test to test non empty arrays and i have changed this to test undefined
| const obj3 = { | ||
| title: "Things fall apart", | ||
| year: 1958, | ||
| publisher: "pan-macmillan", | ||
| }; | ||
| const propertyNameCheck3 = "location"; | ||
| const expected3 = false; | ||
| const result3 = contains(obj3, propertyNameCheck3); | ||
| expect(result3).toBe(expected3); |
There was a problem hiding this comment.
Is it necessary to add a suffix 3 to the names of these local variables?
There was a problem hiding this comment.
It is'nt, the variables are in different scopes, i have removed the suffixes
d-odumosu
added
Needs Review
I think i have fixed all suggested changes |
| if (pair.includes("+")) { | ||
| pair = pair.replaceAll("+", " "); | ||
| } |
There was a problem hiding this comment.
- Could also just call
.replaceAll()without the if statement.
cjyuan
added
Complete
2 participants
Self checklist
Changelist