Skip to content

GLASGOW | Jan-26-ITP | Prakash Dcosta | Sprint 1 | Data Groups#1005

Closed
dcostaprakash wants to merge 3 commits intoCodeYourFuture:mainfrom
dcostaprakash:Sprint-1
Closed

GLASGOW | Jan-26-ITP | Prakash Dcosta | Sprint 1 | Data Groups#1005
dcostaprakash wants to merge 3 commits intoCodeYourFuture:mainfrom
dcostaprakash:Sprint-1

Conversation

@dcostaprakash
Copy link
Copy Markdown

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the [style guide]

Changelist

In this PR, i have implemented functions according to requrements and built test cases

Self checklist

- [X] I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
- [X] My changes meet the requirements of the task
- [X] I have tested my changes
- [X] My changes follow the [style guide]

## Changelist

In this PR, i have implemented functions according to requrements and built test cases
@dcostaprakash dcostaprakash added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 13, 2026
Comment thread Sprint-1/fix/median.js
Comment thread Sprint-1/implement/dedupe.test.js
Comment thread Sprint-1/implement/max.test.js Outdated
Comment thread Sprint-1/implement/sum.js Outdated
Comment thread Sprint-1/implement/sum.test.js
@cjyuan cjyuan 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. labels Mar 19, 2026
@dcostaprakash
Copy link
Copy Markdown
Author

Made changes as per the review. Thank you

@dcostaprakash dcostaprakash added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Mar 20, 2026
Comment thread Sprint-1/fix/median.js
if (numbers.length === 0) return null;

// Sort numbers without modifying original list
const sorted = [...numbers].sort((a, b) => a - b);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it necessary to make a copy of numbers before sorting?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No, it is not necessary to make a copy of numbers before sorting, however, it is a good practice.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you elaborate how it is a good practice in this scenario?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In this scenario it is not needed. I just tried to make it cleaner by removing non numeric values if any. Do you want me to remove it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"... to make it cleaner by removing non numeric values" -- You have already removed non numeric values using .filter().

You can keep the code unchanged. I am more interested in knowing why you think "making a copy of numbers before sorting" is a good practice. Can you explain?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok. I got your point. According to me, making a copy before sorting is a good practice for the following reasons:

  1. We could accidentally modify it for example if we use the median function directly, the new file would be sorted and it would replace the original file
  2. Incase we need to refer the original file later in the code, we will not be able to since the index of the original file could get sorted
    I hope this answers. If i have missed anything, pl add to it. Thank you

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • By "file", do you mean "array"?

  • It's true that numbers.sort() can mutate the numbers array. If we need to use numbers after sorting, then it is considered a safe practice to not mutate the array. In your implementation, is numbers being used after sorting?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

• Yes by file I meant array.
• No, in my current implementation code, the numbers array is not used again after the sorted variable is created.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"immutability" do has its advantage. The reasons you gave do not quite fit the numbers array as the array is clearly not needed after sorting.

Thought you might be interested in this ChatGPT explaination:
https://chatgpt.com/share/69cf697a-5ea8-8331-bb33-47ccc8b02a17

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok. I will look at it in detail. Thank you

Comment thread Sprint-1/implement/dedupe.test.js
Comment thread Sprint-1/implement/max.test.js
@cjyuan cjyuan 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. labels Mar 20, 2026
@dcostaprakash
Copy link
Copy Markdown
Author

Have made the changes as per the feedback. Thank you

@dcostaprakash dcostaprakash added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Mar 24, 2026
@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Mar 24, 2026

Can you address this follow-up comment: #1005 (comment)

image

Other changes are good.

@cjyuan cjyuan 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. labels Mar 24, 2026
@dcostaprakash dcostaprakash added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Apr 2, 2026
@cjyuan cjyuan 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. labels Apr 2, 2026
@dcostaprakash dcostaprakash added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Apr 3, 2026
@cjyuan cjyuan 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. labels Apr 3, 2026
@dcostaprakash dcostaprakash added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Apr 3, 2026
@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 3, 2026
@cjyuan cjyuan added the Complete Volunteer to add when work is complete and all review comments have been addressed. label Apr 3, 2026
@illicitonion
Copy link
Copy Markdown
Member

Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants