ITP_GLASGOW_APR | HANNA_MYKYTIUK | MODULE__DATA_\GROUPS | SPRINT_2 #492
Conversation
There was a problem hiding this comment.
Please remove unrelated files when you request a code review.
| it("in case of invalid parametr function contains should throw an error", () => { | ||
| expect(contains( [1,2,3], 'property')).toEqual(false); | ||
| expect(contains( "test", 'property')).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
1st Issue:
Did you run this test script? I don't think your current implementation can pass this test.
Array is a kind of objects in JavaScript where its indexes are treated as key. So
contains([4, 4, 4], '0') or contains([4, 4, 4], 1) would return true.
Object.hasOwn("ABC", 1) would also return true.
If you want to reject any type of value that is not an object, you will need to explicitly write code to check. (You can try asking ChatGPT how to properly check if a value is an object in JS)
2nd Issue:
Your function is not throwing any error. If you modify your function to throw error, you would need to prepare the test code in the following manner:
it("in case of invalid parameter function contains should throw an error", () => {
// Need to wrap the function call in a function
expect( () => contains([1,2,3], 'property') ).toThrow();
expect( () => contains("test", 'property') ).toThrow();
});
| let value = ''; | ||
| for(let i = 1; i<array.length; i++){ | ||
| value += array[i]; | ||
| if (i != array.length - 1){ | ||
| value += '='; | ||
| } | ||
| } |
There was a problem hiding this comment.
You can also write this as array.slice(1).join("="). Return an array without the first element and then join the elements using '='.
| value += '='; | ||
| } | ||
| } | ||
| queryParams[array[0]] = value; |
There was a problem hiding this comment.
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() {} | ||
| function tally(array) { | ||
| try { | ||
| if (typeof(array) == 'string'){ |
There was a problem hiding this comment.
There is a way to check if a value is an array. (Ask ChatGPT)
You can just return false or throw an error if array is NOT an array, instead of just check for "string". There are other types of value that is not array.
| let copy = []; | ||
| for (let i=0; i<array.length; i++){ | ||
| if (copy.indexOf(array[i]) === -1) { | ||
| copy.push(array[i]); | ||
| } | ||
| } | ||
| let object = {}; | ||
| for (let i=0; i < copy.length; i++){ | ||
| let count = 0; | ||
| for (let j=0; j < array.length; j++){ | ||
| if (array[j] == copy[i]){ | ||
| count = count + 1; | ||
| } | ||
| } | ||
| object[copy[i]] = count; | ||
| } |
There was a problem hiding this comment.
Would you try implement the following approach (which is more efficient by using object as a lookup table):
let object = {};
for each element, ele, in the arrays
if object[ele] is undefined
set object[ele] to 1
else
increment object[ele] by 1
| it("Given an invalid input it should throw an error", () => { | ||
| expect(tally('string')).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
Your function is not throwing any error when the parameter is not an array (the description of the test is not quite correct).
| noPunctuation = noPunctuation.toLowerCase(); | ||
| const words = noPunctuation.split(" "); | ||
| for(i=0; i<words.length; i++){ | ||
| if (Object.hasOwn(countedWords, words[i])){ |
There was a problem hiding this comment.
We could also write if (countWords[words[i]]), which can be interpreted as "if countWords[words[i]] has a value" (and we know that it cannot be zero).
cjyuan
added
Reviewed
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.