-
-
Notifications
You must be signed in to change notification settings - Fork 200
London|25-ITP-September|Alexandru Pocovnicu|Sprint 2 #814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
London|25-ITP-September|Alexandru Pocovnicu|Sprint 2 #814
Conversation
… properties and invalid parameters
… with input validation
…rs and key-value parsing
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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). |
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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). |
4 similar comments
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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). |
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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). |
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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). |
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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). |
takanocap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexandru-pocovnicu Good effort on Sprint 2. However, there a few comments for you to address.
Sprint-2/debug/recipe.js
Outdated
| console.log(`${recipe.title}, serves ${recipe.serves}, ingredients:`); | ||
|
|
||
| for (const ingredient of recipe.ingredients) { | ||
| console.log(ingredient); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be done with an array method which may be simpler/apt in this scenario?
Sprint-2/implement/contains.js
Outdated
| if (typeof obj !== "object" || obj === null || Array.isArray(obj)) { | ||
| throw new Error("Parameter is not an object literal"); | ||
| } | ||
| if (Object.keys(obj).length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to make fewer checks so we you don't repeat the same check logically?
Sprint-2/implement/contains.js
Outdated
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way we can reduce repetition? what is the DRY principle?
| // implementation here | ||
| function createLookup(countryCurrency) { | ||
| let countryCurrencyPairs = {}; | ||
| for (const pair of countryCurrency) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if you test this solution with an argument which is not an object?
Sprint-2/implement/querystring.js
Outdated
| @@ -1,16 +1,32 @@ | |||
| function parseQueryString(queryString) { | |||
| const queryParams = {}; | |||
| if (queryString.length === 0) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a better way to check this and exit or handle the error if its not a valid querystring?
Sprint-2/implement/querystring.js
Outdated
| for (const pair of keyValuePairs) { | ||
| const [key, value] = pair.split("="); | ||
| queryParams[key] = value; | ||
| if(pair===""){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be shorter or tested in a different way ? investigate how to test if string has a empty or falsy value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so should i change my tests also? if i use array destructuring none of the test are passing anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please because thats what will typically happen in a work environment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i use array destructuring it doesn't pass the tests as some of the values have "=" in them, or at least i don't know how to do it. some pointers would be much appreciated , thank you
…dling key-value pairs
| test("if the object contains the property, return 'true'", () => { | ||
| expect(contains({ a: 1, "a,s,3": "op" }, ["a", "s", 3])).toEqual(true); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is slightly surprising behaviour - the object doesn't really contain that array, it contains a stringification of that array. But this works, so I guess it is ok to test for...
I would probably use a different name for the test, though, to make it clear how it's different from the test on line 19.
| test("throw error if the argument is not an object", () => { | ||
| expect(() => createLookup("notAnObject")).toThrow( | ||
| "Argument must be an array" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit odd that the test name here talks about objects, the string says notAnObject, but the error message is about being an array?
| //[[ "a", 1], ["b", 2]] | ||
|
|
||
| // c) What is the target return value when invert is called with {a : 1, b: 2} | ||
| //{"1":a,"2":b} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the type of a and b here? Are they strings/numbers/booleans/variables/something else?
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. 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. |
1 similar comment
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. 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. |
Learners, PR Template
Self checklist
Changelist
implemented functions using TDD