London | 26-ITP-Jan | Mohsen Zamani | sprint 2 | coursework#953
London | 26-ITP-Jan | Mohsen Zamani | sprint 2 | coursework#953mohsenzamanist wants to merge 4 commits intoCodeYourFuture:mainfrom
Conversation
Sprint-2/implement/contains.test.js
Outdated
| expect(() => contains(undefined, "key1")).toThrow( | ||
| "The parameter given is not a plain JS object." | ||
| ); | ||
| expect(() => contains(null, "key1")).toThrow( | ||
| "The parameter given is not a plain JS object." | ||
| ); |
There was a problem hiding this comment.
Number is not a plain JS object but it is given a different treatment.
Wouldn't it be "friendlier to the caller" if the function can be designed to behave consistently for any value that is not an object or is an array?
| function countWords(string) { | ||
| const noPunctuationStr = string.replace(/[.,!?]/g, ""); | ||
| const wordArray = noPunctuationStr.split(" "); | ||
| let wordCount = new Map(); | ||
| for (let word of wordArray) { | ||
| wordCount.set(word, (wordCount.get(word) || 0) + 1); | ||
| } | ||
| const sortedWordCount = [...wordCount.entries()].sort((a, b) => b[1] - a[1]); | ||
| return sortedWordCount; |
There was a problem hiding this comment.
Can you check if your function returns what you expect in the following function calls?
countWords("Hello,World! Hello World!");
countWords(" Hello World ");
Sprint-2/implement/contains.js
Outdated
| @@ -1,3 +1,9 @@ | |||
| function contains() {} | |||
| function contains(obj, property) { | |||
| if (Object.prototype.toString.call(obj) !== "[object Object]") return false; | |||
There was a problem hiding this comment.
This approach to check if obj is an object is not 100% accurate. For example,
const map = new Map()
console.log(map instanceof Object); // true
console.log(Object.prototype.toString.call(map)) // [object Map]map is an object but your approach would not recognise it as an object.
This test description in contains.test.js is a bit vague because an array is a kind of object in JavaScript:
// Given invalid parameters like an array
// When passed to contains
// Then it should return false or throw an error
Can you interpret "invalid parameter" as "it is not an object OR it is an array"?
There was a problem hiding this comment.
Does the code only need to check the plain objects (excluding Map)?
There was a problem hiding this comment.
I do not know the intent of the person who wrote the test description. My interpretation is, accept all objects except arrays.
There was a problem hiding this comment.
The approach you were using is not a reliable way to check for pure Object. Without a more specific description of what is expected of such object, let's just practice on "accept any object that is not an array"
Sprint-2/stretch/count-words.js
Outdated
| const sortedWordCount = new Map([...wordCount].sort(([, a], [, b]) => b - a)); | ||
| return sortedWordCount; |
There was a problem hiding this comment.
-
The syntax of the expected output on line 13 seems to suggest the return value should be a plain JS object instead of a Map object.
-
On line 39, it would make the statement more readable if
aandbare replaced by more meaningful identifiers.
cjyuan
left a comment
There was a problem hiding this comment.
Could you remove the debugging code to keep the code clean?
Sprint-2/stretch/count-words.js
Outdated
| .split(/[.,!? ]+/) | ||
| .filter(Boolean); | ||
| let wordCount = new Map(); | ||
| const wordCount = {}; |
There was a problem hiding this comment.
Note: You could just change the code on line 39 and keep wordCount as a Map object.
Sprint-2/implement/contains.js
Outdated
| if (Object.prototype.toString.call(obj) !== "[object Object]") return false; | ||
|
|
||
| if (Array.isArray(obj) || obj === null || typeof obj !== "object") { | ||
| console.log("here"); |
There was a problem hiding this comment.
Debugging code should be removed to make the code cleaner.
Learners, PR Template
Self checklist
Changelist
Complete Sprint 2 exercises.
Questions