Skip to content

Conversation

@Elfredah
Copy link

@Elfredah Elfredah commented Mar 30, 2025

I deleted the other sprint to ensure I submit the sprint 1 separately<!--

-->

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

@day-lee day-lee added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Apr 5, 2025
@day-lee day-lee self-requested a review April 5, 2025 19:36
@day-lee
Copy link

day-lee commented Apr 5, 2025

Hi, @Elfredah. Please make sure to add the label needs review so it shows up on the volunteers' dashboard. You can get a quicker review once you add a label to your PR.

module.exports = calculateMedian;


//======> solution to the right codes to solve the issue is below
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good and passes the test.
One thing I want to mention is that when you share the code with others, please provide the final version to avoid any confusion. If you need the code for future reference, you can comment it out and share it, but the best practice would be to remove any unnecessary code before PR.

Comment on lines +38 to +59
//const calculateMedian = require("./median.js");

//describe("calculateMedian", () => {
//test("returns the median for odd length array", () => {
//expect(calculateMedian([1, 2, 3])).toEqual(2);
//expect(calculateMedian([1, 2, 3, 4, 5])).toEqual(3);
//});

//test("returns the average of middle values for even length array", () => {
//expect(calculateMedian([1, 2, 3, 4])).toEqual(2.5);
//expect(calculateMedian([1, 2, 3, 4, 5, 6])).toEqual(3.5);
//});

//test("doesn't modify the input", () => {
//const list = [1, 2, 3];
//calculateMedian(list);

//expect(list).toEqual([1, 2, 3]);
//});
//});

//=======> solution to the right codes to solve the issue is below
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete any unnecessary commented-out lines.

Comment on lines +61 to +77
// median.js (the file containing calculateMedian)
function calculateMedian(list) {
if (!list || list.length === 0) {
return undefined;
}

const sortedList = [...list].sort((a, b) => a - b);
const middleIndex = Math.floor(sortedList.length / 2);

if (sortedList.length % 2 === 0) {
return (sortedList[middleIndex - 1] + sortedList[middleIndex]) / 2;
} else {
return sortedList[middleIndex];
}
}

module.exports = calculateMedian;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The est.js file should only contain the test suite. Since the function is already written in median.js so you can delete those lines.

function dedupe() {}
//function dedupe() {}
// dedupe.js
function dedupe(arr) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! As a bonus challenge, can you think of a solution that uses Set object as well?

// When passed to the max function
// Then it should return the least surprising value given how it behaves for all other inputs

const findMax = require("./max");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you run the test with the npm test <file path> command? I tried it on my local machine, and it fails. I noticed that you wrote this line of code twice in the file.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to delete one of those duplicate const findMax = require("./max"); line to make it work.


module.exports = findMax;
//module.exports = findMax;
function findMax(elements) {
Copy link

@day-lee day-lee Apr 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With current code, test fails under these conditions.

given an empty array, returns -Infinity
given an array with non-number values, returns the max and ignores non-numeric values
given an array with only non-number values, returns -Infinity 

You need a line of code that checks the type of element in an array(elements) and find max from filteredArray. Can you add the line to filter only the numbers?

function findMax(elements) {
// Return undefined if the array is empty
if (elements.length === 0) {
return undefined;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the test, it specifies that it needs to return -Infinity, not undefined. This needs to be changed. Additionally, you should check the length of filteredArray because if there are only strings in the array, it means the array has no valid numbers and is effectively an empty array.

test("given an empty array, returns -Infinity", () => {
    expect(findMax([])).toEqual(-Infinity);
  });

}

// Use Math.max to find the largest number in the array
return Math.max(...elements);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job using Math.max() function with spread operator to spread the elements from an array.

function sum(elements) {
}

module.exports = sum;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be deleted to run the test as it's duplicate and cause an error.

// Test for an array with decimal/float numbers
test("given an array with decimal/float numbers, returns the correct total sum", () => {
expect(sum([1.5, 2.5, 3.5])).toEqual(7.5); // 1.5 + 2.5 + 3.5 = 7.5
expect(sum([1.1, -0.2, 3.2])).toEqual(4.1); // 1.1 + (-0.2) + 3.2 = 4.1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I run the test, this test fails as sum([1.1, -0.2, 3.2] returns 4.1000000000000005. This is because JavaScript has one numeric type which is float. This can be not precise and has a rounding issue. Can you think of a solution using parseFloat() and toFixed() function? Check if your final code passes all test as well.

@day-lee day-lee added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Apr 5, 2025
@day-lee
Copy link

day-lee commented Apr 5, 2025

Great job on completing Sprint 1! You did well overall. I've commented on a few minor issues. You can run the test using npm test <path/filename.test.js>. If you have any more questions, feel free to ask! :)

@Elfredah Elfredah closed this by deleting the head repository May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants