Skip to content

Conversation

@donRehan
Copy link
Contributor

fix ambiguity in test description , old was 'finds the person with the greatest age if someone is still living.'
remove redundant test that checks for 'finds the person with the greatest age if the OLDEST is still living' since both this and the above test for ability to check if there is no death year field.

Because

I had faced difficulty understanding the tests so wanted to make it easier for others to understand the test when they are learning

This PR

  • Remove one of the three tests as I think it is redundant
  • Modify the test description of the last test to make it more concise

Issue

Closes #XXXXX

Additional Information

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the location of change: brief description of change format, e.g. 01_helloWorld: Update test cases
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If this PR includes any changes that affect the solution of an exercise, I've also updated the solution in the /solutions folder

fix ambiguity in test description , old was 'finds the person with the
greatest age if someone is still living.'
remove redundant test that checks for 'finds the person with the
greatest age if the OLDEST is still living' since both this and the
above test for ability to check if there is no death year field.
@UniquePixels
Copy link

These tests actually check two seperate cases. In the first case it verifies that the function returns the oldest person when there is a person still living in the set. In the second case it makes sure that the function returns the oldest person when there is a person still living in the set AND that person is still living.

Why would you need both checks? Perhaps someone would write a function that simply skips living people completely and therefore misses a case. Or does not compute the living person correctly...etc.

@donRehan
Copy link
Contributor Author

According to what I understand from you. We check for these two cases.

Oldest including living people. Oldest is the living person.

This means we want to check if the code in the two cases

doesn't Skip living people.
doesn't Skip dead people.

Then I would like to change the comment to be as stated above.
I will be adjusting it for better readability once I have your confirmation.

@thatblindgeye
Copy link
Contributor

@donRehan just checking if you're able to make the updates as discussed in the above comment?

@donRehan
Copy link
Contributor Author

donRehan commented Feb 12, 2024

@thatblindgeye

I apologize this is my first PR didn't know how to change it, but I believe I fixed it as per our discussion now.

@thatblindgeye thatblindgeye merged commit b092163 into TheOdinProject:main Feb 13, 2024
Oussama5379 added a commit to Oussama5379/javascript-exercises that referenced this pull request Feb 1, 2025
12_findTheOldest fix ambiguity and remove redundant test description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants