-
-
Notifications
You must be signed in to change notification settings - Fork 156
WESTMIDLANDS | ITP-MAY-25 | Ahmad Ehsas | Sprint 2| DATA GROUPS #756
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
base: main
Are you sure you want to change the base?
Conversation
…rray and passed the tests of given arrays.
…seNumber property.
…by join(\n) method.
It looks like there are a lot of files included here - from multiple sprints. Can I just confirm which ones you wanted reviewed? |
Subject: Request for Review of Sprinter-2 Folder
I hope this message finds you well. I would like to request a review of the
Sprinter-2 folder, as it is pertinent to my branch, Sprint-2.
Thank you for your attention to this matter.
*LonMcGregor* left a comment (CodeYourFuture/Module-Data-Groups#756)
… <#756 (comment)>
It looks like there are a lot of files included here - from multiple
sprints. Can I just confirm which ones you wanted reviewed?
—
Reply to this email directly, view it on GitHub
<#756 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BOYJJVAGCTH5O66MQ2LMQBL3NBLUFAVCNFSM6AAAAACDP5QJCWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCNZTHAZTMMBYHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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.
Hi, I've reviewed sprint 2 now. If the sprint 1 files are not meant to be there, can you remove them from the pull request please? Also, can you fix the package-lock.json
file - if you didn't intentionally change that file, it should not have any changes comitted.
For sprint 2, you have made a good start, but I have some follow up questions for you to answer
// This means that we need to check if the property belongs to the object using hasownproperty whenever we loop through an object with the `for ... in` loop. | ||
|
||
for (const key in author) { | ||
if (author.hasOwnProperty(key)) { |
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 are you using hasOwnProperty to do here?
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.
The Sprints 1, 2 and 3 files are in the same folder, and my Sprint-1 has already been reviewed. Is it necessary to remove it?
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.
hasOwnproperty is being used to check whether the key belongs directly to the author object itself, not to something it inherited from its prototype chain.
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.
Thanks for answering about hasownproperty. For the files in the other sprints, if they are not relevant to this pull request, they should be removed from the branch.
(hint: the files should be restored to original state note deleted entirely)
${recipe}`); | ||
// Here we add `join("\n") to the ingredients array to log each ingredient in a new line. | ||
console.log( | ||
`${recipe.title} serves ${ |
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 reason you wrote the recipe.serves
variable over multiple lines?
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.
Breaking recipes into multiple lines just makes the final string output easier to read when you look at the code.
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 meant the code itself on line 16 - is there a reason you wrote the variable recipe.serves
in the template expression over multiple lines instead of one line?
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 think there is no functional reason for splitting recipe. serves
over multiple lines. If we write in one line or multiple lines, it doesn't change the output.
|
||
// Given invalid parameters like an array | ||
// When passed to contains | ||
// Then it should return false or throw an error | ||
test("contains on an array returns false", () => { | ||
expect(contains([1, 2, 3], 4)).toBe(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.
What happens if you test for something like contains([1,2,3,4], "length")
?
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.
The test returns true because 'length' is a property of the array object
|
||
// d) Explain why the current return value is different from the target output | ||
// because it returns the [key, value] of an object in reverse order [value,key] |
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 think this question was asking why the original output of this program was not what was expected (before you fixed it) - can you explain why you made the change you did and what the mistake was?
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.
Because it was required to fix the implementation and write tests to prove it is fixed. I followed the instructions and made the required changes. There were a lot of mistakes at first when I wanted to check the test, and there were errors.
Thanks for answering my questions. To review what still needs to be addressed here:
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (SPRINT-2) doesn't match expected format (example: 'Sprint 2', without quotes) |
1 similar comment
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (SPRINT-2) doesn't match expected format (example: 'Sprint 2', without quotes) |
SPRINT-2
| DATA GROUPS
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part ( |
SPRINT-2
| DATA GROUPS
Learners, PR Template
Self checklist
Changelist:
Completed all the mandatory tasks.
Questions
Ask any questions you have for your reviewer.