-
-
Notifications
You must be signed in to change notification settings - Fork 286
Manchester | 26-ITP-Jan | Mehroz Munir | Sprint 1 | Coursework exercises #1018
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,8 +6,16 @@ | |
| // or 'list' has mixed values (the function is expected to sort only numbers). | ||
|
|
||
| function calculateMedian(list) { | ||
| const middleIndex = Math.floor(list.length / 2); | ||
| const median = list.splice(middleIndex, 1)[0]; | ||
| let median = null; | ||
| if (Array.isArray(list) && !list.every((item) => typeof item != "number")) { | ||
| numericList = list.filter((item) => typeof item === "number"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, something about the declaration of these variables
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh yes, I now understand the scope better after reading this. Thanks. |
||
| sortedList = numericList.toSorted((a, b) => a - b); | ||
| const middleIndex = Math.floor(sortedList.length / 2); | ||
| if (sortedList.length % 2 === 0) { | ||
| const medianArray = sortedList.splice(middleIndex - 1, 2); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's totally right. I have changed the code here to not use splice method and instead use the array index to access the values. |
||
| median = (medianArray[0] + medianArray[1]) / 2; | ||
| } else median = sortedList.splice(middleIndex, 1)[0]; | ||
| } | ||
| return median; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, organised return statement. If the above code falls through, this will catch it.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it can't be null. I have added a check at the end for that. Let me know if it seems right. Thanks |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,12 @@ | ||
| function dedupe() {} | ||
| function dedupe(arr) { | ||
| if (arr.length === 0) return arr; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice length check here 👍 But could the array just not be present, i.e. null/undefined?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yes, I thought about these null/ undefined checks when working on these test files, but as this case was not described already, I thought we might not be supposed to do this. But I think it is always a good practice to have those checks. I have added them now. Kindly check, please. |
||
| else { | ||
| dedupeArray = []; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if |
||
| arr.forEach((element) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is generally good logic. Well done 👍
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for introducing Set into my life. I am using them now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hehe, happy to do so. But ensure you follow the goals of the current chapter. There's always another advanced technique, but follow the goal/description of the task, even if it's painful to do so. |
||
| if (!dedupeArray.includes(element)) dedupeArray.push(element); | ||
| }); | ||
| return dedupeArray; | ||
| } | ||
| } | ||
|
|
||
| module.exports = dedupe; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,18 +10,41 @@ E.g. dedupe(['a','a','a','b','b','c']) target output: ['a','b','c'] | |
| E.g. dedupe([5, 1, 1, 2, 3, 2, 5, 8]) target output: [5, 1, 2, 3, 8] | ||
| E.g. dedupe([1, 2, 1]) target output: [1, 2] | ||
| */ | ||
| describe("dedupe", () => { | ||
| // Given an empty array | ||
| // When passed to the dedupe function | ||
| // Then it should return an empty array | ||
| it("given an empty array it should return an empty array", () => { | ||
| const array = []; | ||
| dedupeArray = dedupe(array); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this one slipped through the crack. Let's add a keyword |
||
| expect(dedupeArray).toEqual([]); | ||
| }); | ||
|
|
||
| // Acceptance Criteria: | ||
|
|
||
| // Given an empty array | ||
| // When passed to the dedupe function | ||
| // Then it should return an empty array | ||
| test.todo("given an empty array, it returns an empty array"); | ||
|
|
||
| // Given an array with no duplicates | ||
| // When passed to the dedupe function | ||
| // Then it should return a copy of the original array | ||
|
|
||
| // Given an array with strings or numbers | ||
| // When passed to the dedupe function | ||
| // Then it should remove the duplicate values, preserving the first occurence of each element | ||
| // Given an array with no duplicates | ||
| // When passed to the dedupe function | ||
| // Then it should return a copy of the original array | ||
| [["a", "b", "c"], ["A", 1, "j", "?"], ["c"]].forEach((val) => | ||
| it(`returns copy of the original array if there are no duplicates in [${val}]`, () => | ||
| expect(dedupe(val)).toEqual(val)) | ||
| ); | ||
| // Given an array with strings or numbers | ||
| // When passed to the dedupe function | ||
| // Then it should remove the duplicate values, preserving the first occurrence of each element | ||
| [ | ||
| { input: [1, 2, 1, 3, 1, 2, 10, 5, 0, 10], expected: [1, 2, 3, 10, 5, 0] }, | ||
| { input: [1, 2, 1, 4], expected: [1, 2, 4] }, | ||
| { input: [1, 1, 1, 1, 1], expected: [1] }, | ||
| { | ||
| input: ["banana", "apple", "apple", "banana", "apple", "banana"], | ||
| expected: ["banana", "apple"], | ||
| }, | ||
| { | ||
| input: [" ", "empty", "", " ", "", "empty"], | ||
| expected: [" ", "empty", ""], | ||
| }, | ||
| { input: ["2", "2", "3", "1"], expected: ["2", "3", "1"] }, | ||
| ].forEach(({ input, expected }) => | ||
| it(`returns a copy of array removing the duplicates from [${input}]`, () => | ||
| expect(dedupe(input)).toEqual(expected)) | ||
| ); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| function findMax(elements) { | ||
| function findMax(array) { | ||
| numbersArray = array.filter((value) => typeof value === "number"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent type check. But also ensure to do null/undefined check |
||
| return Math.max(...numbersArray); | ||
| } | ||
|
|
||
| module.exports = findMax; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,10 @@ | ||
| function sum(elements) { | ||
| function sum(array) { | ||
| numbersArray = array.filter((value) => typeof value === "number"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, null/undefined check should be added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, look into
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I read about reduce, and it seems better to use that here. |
||
| let sum = 0; | ||
| numbersArray.forEach((element) => { | ||
| sum += element; | ||
| }); | ||
| return sum; | ||
| } | ||
|
|
||
| module.exports = sum; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,8 @@ | ||
| // Refactor the implementation of includes to use a for...of loop | ||
|
|
||
| function includes(list, target) { | ||
| for (let index = 0; index < list.length; index++) { | ||
| const element = list[index]; | ||
| if (element === target) { | ||
| return true; | ||
| } | ||
| for (const element of list) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, you must use a for-loop, I see. Otherwise, |
||
| if (element === target) 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.
Excellent check of valid types for each item in an array. 👍
There are a few things to improve here:
You're doing a
double negative checkhere. One positive check would reduce the complexity.Use the
early returnmethod, where if your data doesn't match a certain criterion, you just stop with either a return or an error. More: https://gomakethings.com/the-early-return-pattern-in-javascript/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.
Thank you so much, that was really helpful. I have implemented this now.