-
-
Notifications
You must be signed in to change notification settings - Fork 220
London | ITP-Sep-25 | Gislaine Della Bella | Data Groups | Sprint- 1 #810
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
Conversation
// It is not my solution I got help with AI. I confess I need to undestand more those 3 methodos to think how to get to this. // I will come back to it later. For now i will keep practicing more katas because they are help me.
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes) 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). |
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 (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes) 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: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes) 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: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes) 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). |
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 (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes) 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). |
ckirby19
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.
Great work, mostly comments on formatting. Let me know if you are stuck on anything
| // } | ||
|
|
||
| // Ignore this Commit | ||
| // It is not my solution I got help with AI. I confess I need to undestand more those 3 methodos to think |
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 appreciate your honesty :D
| } | ||
|
|
||
| //Filter non-numeric values and create a new array of only numbers | ||
|
|
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.
Generally we don't leave a line between the comment and the code the comment is related to
| } | ||
|
|
||
| //Sort the numeric list | ||
| const sortedList = [...numericList].sort((a, b) => a - 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.
Can you explain why [...numericList] is needed? What would happen if I did numericList.sort instead?
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 is better not to change the original array in case I want to use it later in my code. I'm sorting a "copy" of it.
| @@ -1 +1,13 @@ | |||
| function dedupe() {} | |||
| function dedupe(arr) { | |||
| let result = []; | |||
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.
Does result have to be let here, or can it be const?
Same for item - can it be const?
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 got it now. result should be const because the reference to the array doesn't need to be changed. For item in the , it can also be const, as a new item variable is declared for each iteration, because it's not reassigned within a single loop cycle.
| // Given an array with no duplicates | ||
| // When passed to the dedupe function | ||
| // Then it should return a copy of the original array | ||
| test(" Given an array with no duplicates", () => { |
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.
No need for the space between " and G
| // Then it should return that number | ||
| test("given an array with 1 number, it returns the number ",() => {; | ||
| const inputMax1 = [3]; | ||
| const result = findMax(inputMax1); |
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.
Formatting: ensure all lines have the same indentation. Same for all tests below
|
|
||
| let onyNumbers = theArray.filter(item => typeof item === 'number'); | ||
|
|
||
|
|
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.
Formatting:
- Misspelling of onyNumbers
- We usually don't leave two empty lines between code like this
| function sum(elements) { | ||
| function sum(theArray) { | ||
|
|
||
| let onyNumbers = theArray.filter(item => typeof item === 'number'); |
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.
Does this need to be let?
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.
No, It can be const because the variable is assigned once with the filtered array.
| // When passed to the sum function | ||
| // Then it should return that number | ||
|
|
||
| test("given an array with 1 number, it returns the sam number", () => { |
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.
Misspelling: sam
|
|
||
|
|
||
| function includes(list,target){ | ||
| for (let value of list) { |
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.
Does this need to be let?
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 start to undestand now these const and let .. i thought let would always give me less risk of error, but I can see the opposite now.
thank you for your reviwes ;)
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes) 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). |
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 (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes) 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). |
|
I start to undestand now these const and let .. i thought let would always give me less risk of error, but I can see the opposite now. thank you for your reviwes ;) |
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes) 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). |
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 (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes) 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). |
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.