West Midlands | ITP-Sept-25 | Mustaf Asani | Sprint 1 | Data groups coursework/sprint 1 #847
Conversation
asaniDev
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Groups/blob/sprint-1-feedback/Sprint-1/feedback.md
Doing so can help reviewers speed up the review process. Thanks.
cjyuan
added
Reviewed
asaniDev
added
the
Needs Review
| return (numberArray[middleIndex] + numberArray[middleIndexPlus1]) / 2; | ||
| } | ||
|
|
||
| return numberArray.splice(Math.floor(numberArray.length / 2), 1)[0]; |
There was a problem hiding this comment.
Can you simplify this statement (line 57)?
| let middleIndex = 0; | ||
| let middleIndexPlus1 = 0; | ||
|
|
||
| if (numberArray.length % 2 === 0) { | ||
| //if array has even number of items then a different median formula | ||
| middleIndex = Math.floor(numberArray.length / 2); | ||
| middleIndexPlus1 = middleIndex - 1; | ||
|
|
||
| return (numberArray[middleIndex] + numberArray[middleIndexPlus1]) / 2; | ||
| } |
There was a problem hiding this comment.
If these two variables are used only inside the if block, it is better to declare them inside the block.
| if (Array.isArray(list)) { | ||
| //checking if we have been passed an array before picking numbers from any arrays | ||
| numberArray = list.filter(isNumber); | ||
| console.log(`new array ${numberArray}`); | ||
| } else { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
- Since the function will return immediately if
listis not an array, we could simplify the code as:
if (!Array.isArray(list)) return null;
// no need else and no need to introduce {...} block
...
- It's best practice to keep the code clean by removing all the debugging code. Can you remove all the unnecessary
console.log()statements?
| function isNumber(value) { | ||
| if (typeof value === "number") { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
We could simplify
if (condition)
return true;
return false;
into
return condition; // Note: Use !!(condition) if condition is not a Boolean-type value
| return []; | ||
| } else if (myArray.length === myArray.filter(checkForDupe).length) { | ||
| //return copy of array if no duplicates | ||
| return myArray; |
There was a problem hiding this comment.
This does not return a copy of myArray.
There was a problem hiding this comment.
This function has several bugs.
You may want to reimplement it following this approach:
- First create a new array to keep only the values you consider "valid".
- Then find the max in the new array.
- When dealing with the elements in this new array, you don't have to check for invalid values.
| for (item of list) { | ||
| const element = item; |
There was a problem hiding this comment.
itemis not delcared.- Code on lines 4 and 5 could further be simplified.
| function numberOfFloat(number) { | ||
| if (Number.isInteger(number)) { | ||
| return 0; | ||
| } else { | ||
| return number.toString().split(".")[1].length; | ||
| } | ||
| } |
There was a problem hiding this comment.
This approach to figure out number of decimal places does not work for all floating point numbers.
- Some floating point numbers can also be represented in scientific format. For examples,
1e-100,3.12e50.
There was a problem hiding this comment.
I have to praise you for going through a great length to implement a sum() function that takes into consideration the number of decimal places in the input array.
| input: ["the ", "quick ", "brown ", "fox ", "jumped"], | ||
| expected: "the quick brown fox jumped", | ||
| }, | ||
| { input: [-1000, -500, -450, -320, -2300, -170, -260], expected: -5000 }, |
cjyuan
removed
the
Needs Review
3 participants
Learners, PR Template
Self checklist
Changelist
Added tests to my tasks before creating functions to pass the tests, completed all tasks.