EMIN AKTURK | BIRMINGHAM | MODULE-DATA-GROUPS - SPRINT 2 #763
Conversation
eminakturk
added
the
Needs Review
There was a problem hiding this comment.
-
Can you revert the changes made in the Sprint-1 folder to keep this branch clean?
-
Why not practice "committing files one by one, on purpose, and for a reason"?
In VSCode, you can select which file to stage, and commit only the staged file.
See: https://www.youtube.com/watch?v=z5jZ9lrSpqk&t=705 (At around 12:50 minute marker, the video shows how to stage a single file).
Sprint-2/implement/contains.js
Outdated
|
|
||
| function contains(obj, prop) { | ||
| if (typeof obj !== 'object' || obj === null || Array.isArray(obj)) return false; | ||
| return Object.prototype.hasOwnProperty.call(obj, prop); |
There was a problem hiding this comment.
Could also call Object.hasOwn(obj, prop);
| test("contains with array input returns false", () => { | ||
| expect(contains([1, 2, 3], "a")).toBe(false); | ||
| }); |
There was a problem hiding this comment.
contains([1, 2, 3], "a") could also return false because "a" is not a property (or key) of [1, 2, 3].
However, "0", "1", "2" are keys of [1, 2, 3], so it is better to specify the test as
expect(contains([1, 2, 3], "1")).toBe(false); (to ensure you are checking what you describe)
| queryParams[key] = value; | ||
| const idx = pair.indexOf('='); | ||
| if (idx > -1) { | ||
| const key = pair.slice(0, idx); |
There was a problem hiding this comment.
Value in key could also be URL encoded.
Sprint-2/implement/querystring.js
Outdated
| if (idx > -1) { | ||
| const key = pair.slice(0, idx); | ||
| const value = pair.slice(idx + 1); | ||
| if (key === "equation") { |
There was a problem hiding this comment.
What is so special about the string "equation"?
Sprint-2/implement/tally.js
Outdated
| const counts = {}; | ||
| for (const item of arr) { | ||
| counts[item] = (counts[item] || 0) + 1; | ||
| } | ||
| return counts; |
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.
cjyuan
added
Reviewed
|
Hello CJ, I'm afraid while I was creating new branches every time, I did not go back to main branch so It created indented branches, that's why It is how it is, I did not fix it for this one because I have to redo everything from beginning which as you know we dont have enough time, I'm dealing with TV project at the same time, If that wouldn't be a big problem for you I would appreciate if you could tag it as completed as if you are happy with the code too. As I said I know where the problem is and won't do it again like that. |
eminakturk
added
the
Needs Review
|
You can use these commands to rebase your This instructions assume you had created a branch named In your case, Important:
1. Open Your Cloned Repository in VSCode and Start a Terminal in VSCode.VSCode will start the terminal in the top-level folder of the current project. 2. Switch to the branch you want to rebase (
|
| @@ -10,10 +10,15 @@ function parseQueryString(queryString) { | |||
| if (idx > -1) { | |||
| const key = pair.slice(0, idx); | |||
| const value = pair.slice(idx + 1); | |||
| if (key === "equation") { | |||
|
|
|||
| if (key === "equation" || key === "formula" || key === "expression" || value.includes('=')) { | |||
There was a problem hiding this comment.
Why threat these cases differently?
cjyuan
added
Complete
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
I am aware of the indented coursework where you could see sprint 1 fixes as well, I'm aware of the situation I will be fixing it with my next module PR.