Skip to content
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

12_findTheOldest: ReadMe Argument Clarification #375

Closed

Conversation

Roberra0
Copy link
Contributor

@Roberra0 Roberra0 commented Jul 8, 2023

Because

The exercise cannot be completed without knowledge of the argument's properties and names. Without knowing that the people array has a birth and death year, or how those properties are referred to, a user cannot write code that passes the test cases. A user has to go into the test case to see examples of people array.

All other exercise readme's have sample code where needed to resolve this.

This PR

  • Updates exercise 12's readMe to include sample code for the argument that will be used to test the users code

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

@Roberra0 Roberra0 changed the title Exercise 12 readme clarification 12_findTheOldest: ReadMe Argument Clarification Jul 8, 2023
@ManonLef
Copy link
Member

ManonLef commented Jul 8, 2023

@Roberra0 It looks like you have made a PR for all the files you have made changes to. To open a PR it's best if you follow the steps in the contributing guide which will include making a branch before you start working on the changes. That way you only submit changes that you intend to include since we don't want to unskip all the tests for example, plus it makes it a lot easier to focus on the actual change.

I'm also going to ask for a second opinion from the @TheOdinProject/javascript team since I believe that a lot of the information regarding the exercises actually does have to be taken from the test cases. As mentioned in the readme for the first exercise:

For now you do not need to worry about how to write tests, but you should try to get comfortable enough with the syntax to figure out what the tests are asking you to do.

Which makes me think this is not a change that's needed. A lot of the tests in the previous exercises, for example the ERROR returning ones, couldn't be passed without looking at the test specs.

Thank you for contributing however, that's highly appreciated whether we do or don't proceed with this change.

@Roberra0
Copy link
Contributor Author

Roberra0 commented Jul 8, 2023

Ah, I see submitted the PR on a branch that has my existing changes. I will keep this open so the JS team can respond, and based on that submit a new PR.

From my perspective, when I reached exercise 12's readme, it felt unfinished. I was used to readme files from the previous exercises giving me enough description on the inputs / outputs to get started coding. All the exercises except "Exercise 08 - Calculator" have examples in the readme on the inputs / outputs to help get started coding without needing you to go in test file first. Exercise 12 is the only exception I see.

I agree with you, users will be in the test file, and can figure it out from there. It would just be nice to include basic info in the readme to get started coding without looking at the test case first.

@wise-king-sullyman
Copy link
Member

@Roberra0 if the readme just reminded learners to reference the test file to understand the argument structure do you think that would've helped prevent any confusion?

@Roberra0
Copy link
Contributor Author

Roberra0 commented Jul 8, 2023

Yes, I think that would help. Even a little guidance like that would go a long way!

Also, sharing a snippet of the previous exercise's readme that I was accustomed to:

Exercise 11 - Get the Titles!

You are given an array of objects that represent books with an author and a title that looks like this:

const books = [
  {
    title: 'Book',
    author: 'Name'
  },
  {
    title: 'Book2',
    author: 'Name2'
  }
]

@wise-king-sullyman
Copy link
Member

I wouldn't block adding this, but I do kind of like having learners go to the tests to get the information.

@Roberra0
Copy link
Contributor Author

Roberra0 commented Jul 9, 2023

Noted, this isn't a big blocker, I was just looking for consistency. Happy to close this or create a new PR properly. Either way, thanks for maintaining the project!

@wise-king-sullyman
Copy link
Member

@Roberra0 thanks for taking the time to contribute to it!

@ManonLef what are your thoughts on next steps here?

@ManonLef
Copy link
Member

ManonLef commented Jul 9, 2023

@wise-king-sullyman I really liked your idea of a heads up in the readme! Especially because it is in line with how TDD works; we first write the test, then the function based on the requirements we gave in the test case.

Perhaps something along the lines of:

"Now that you've reached the end of these exercises, you should be fairly comfortable getting the information you need from the test case(s). Take some time to see how the array of objects is constructed for example and use that information for writing your function"

@Roberra0 Roberra0 mentioned this pull request Jul 11, 2023
6 tasks
@Roberra0
Copy link
Contributor Author

Created a new PR to save you time from editing the file directly! Also because I'm excited to contribute to this project, albeit a minor contribution.

@Roberra0 Roberra0 closed this Jul 11, 2023
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.

None yet

3 participants