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

The width of grades can exceed a single character and may even be non-numeric. #253

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tarekmohamedibrahim
Copy link

While mentored by Filbo, he revealed to me the idea of generalizing the code to accept grades with width more than one character. I checked it to accept also non-numeric grade values. I adjusted my code to pass this test, and I changed one of the testes to do this kind of test.

@glennj
Copy link
Contributor

glennj commented Apr 11, 2024

  1. the tests come from the exercism/problem-specifications repo: changes to existing tests should be discussed there.

  2. each exercise has an example solution to verify that the exercise can be solved: does your proposed test change pass the example solution?

  3. how does a non-numeric grade affect the sorting of grades? Does "AB" come before 1? After any numeric grade? In Ontario, we have junior kindergarten ("JK"), senior kindergarten ("SK") then grade 1. I don't know what "AB" is supposed to mean.

@tarekmohamedibrahim
Copy link
Author

Hi Glenn,
Here are my answers to your questions
2. when I submitted this request, I didn't consider the example solution, but when I ran it, all tests passed, except the one test that I changed, which title is Students are sorted by grades and then by name in the roster, and if I changed the excepted result to be : Barb,Charlie,Alex,Peter,Zoe,Jim,Anna, it would work.
Now, there is a problem introduced, i.e., the sort function "roster_sort", which sorts by grade, then by name, sees the grades as numerals only and it didn't consider the case of Character grades like "JK" or "SK", the function returns : v1-v2 in case of grades, this would be resolved to zero for any passed character grades, and if there are grades added in the text values as SK then JK, they would be seen as the save value when subtracted, i.e KS - SK, as they both would be resolved to zero
So, and this is a question: would you like me to cancel this pull request and change the example solution to accept characters ?

  1. Based on the function "roster_sort", the "AB" comes first. Also, "AB" was supposed by me as just a character and I didn't think while suggesting it for some real grade in life, but may be there are places that use grades like "A", "B" for baby classes

@glennj
Copy link
Contributor

glennj commented Apr 11, 2024

We can still work in the context of this PR. Just add further commits to it.

If we're going to add non-digit grades:

  • it must be done as a new test,
  • the example needs to be updated as required to pass,
  • a .docs/instructions.append.md file should be added to clearly spell out the sorting requirements.
    • Exercism specification for markdown files is to have a single top-level heading.
      However, it is discarded for the append file.
      That means the append file needs to look something like

      # ignore
      
      ## AWK-specific instructions
      
      There are tests with non-numeric grades.
      When generating the roster, these grades need to sort _after_ all numeric grades.

I do appreciate your contribution. Please see the Practice Exercise doc for all the little details about how exercises are constructed within a track.

@glennj
Copy link
Contributor

glennj commented Apr 11, 2024

@IsaacG I'm OK with adding a new test. This exercise gets students to practice sorting and having non-numeric grades adds an interesting new wrinkle to it.

@IsaacG
Copy link
Member

IsaacG commented Apr 11, 2024

  • While we may not validate the input is numeric, I don't think we should be adding non-numeric inputs here.
  • If there is any value to having "wider" grades, I think those should be pushed upstream to the problem specs.

I'm not seeing a justification for these updates to be awk specific and to diverge from the problem specs.

…re sorted by grades and then by name in the roster
@tarekmohamedibrahim
Copy link
Author

tarekmohamedibrahim commented Apr 11, 2024

When I initially created this PR, I was do not aware of the example solution, and I didn't make any effort to change it, which needs new/update specs. What prompted me to change the test "Students are sorted by grades and then by name in the roster", was that my initial code for this problem passed all tests, until Filbo advised me to generalize the code to accept more than one digit. I realized that all tests had only one digit in grades. My code in the last iteration accepted numerals and non-numeric values which gave me the idea to change the test to include non-numeric values.
However, following Issac Good comment, I discarded the idea of introducing non-numeric grades and retained the change that used a grade with two digits ( in some countries, grade numbers before the collage are counted from 1 to 12).

@tarekmohamedibrahim
Copy link
Author

However, I haven't completely abandoned my idea. I might create a new problem that involves sorting both numeric and non-numeric values.

@glennj
Copy link
Contributor

glennj commented Apr 12, 2024

I'm willing to go forward with this, but it has to be a new test not modifying an existing one.

@glennj glennj added the paused Work paused until further notice label Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paused Work paused until further notice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants