Java: Grade-school: Structure very unclear

[colin-mullikin]

The intended structure of the School class is very hard to pin down. All of the add() calls are done directly on the School, but then for the first few tests, things (size, added students) are accessed through an arbitrary middle layer, db(). After the first few tests, suddenly the calls change from school.db().get({grade number}) to school.grade({grade number}).

Examples:
school.add("Chelsea", 3);
assertThat(school.db().get(3).size(), is(1));
assertThat(school.grade(5).size(), is(2));

If the point of the exercise is to force adherence to an arbitrary (and frankly ridiculous) structure, then I would consider it a great success.

[FridaTveit]

I completely agree that this exercise is very confusing at the moment. I agree that the db() layer is not needed, and that using it only some of the time is very confusing. Having looked at the exercise in Java and in C#, I would suggest we remove that .db() function completely and make all the tests use the .grade() syntax. The C# exercise doesn't have the .db function and the description of the exercise doesn't really seem to ask for it. As it's only removing functionality, this shouldn't break any existing solutions.

I also find the name and behaviour of the .sort() function confusing. I would expect a function with that name to internally sort the School object and either return a new School object whose content is now sorted, or to modify the object the function is called on. However, this doesn't quite match what the description of the exercise asks for. As it currently returns the internal database of the School object sorted, I would expect it to be called something like getAllStudentsInAllGradesSorted() or allStudentsInAllGradesSorted(). However, making either of those changes would break existing solutions to the exercise. @colin-mullikin, @stkent, @jtigger and @matthewmorgan what do you think?

The C# and F# exercises have a roster() function which returns all students in all grades (like the db() function). A later test then checks that this dictionary is sorted. I think that implementation makes more sense, though that still means you have two ways of getting all students in a particular grade (school.db().get(grade) and school.grade(grade)).

[colin-mullikin]

I agree with pretty much everything you said.

Regarding the solution breaking change, I don't know if any other tracks do it this way, but the Go track has a const targetTestVersion = {someValue} in the test file, and then you define a const testVersion in your solution to match it. Each test makes sure these are equal; otherwise, it just fails immediately without running the rest of the test. Theirs is also setup a little differently in that as soon as something fails, it stops the whole thing.

I may not have described that in the best manner, so if you'd like more detail just mosey on over to the exercism/xgo repo.

[jtigger]

---

@colin-mullikin: I'm pretty sure we could have done without that last sentence (in your initial comment). To suppose a nonsense basis for the work is disrespectful to the person who did it, even though they are not named outright. It absolutely fine to be critical, let's just keep it respectful, please. Thank you!

---

@FridaTveit: I like where you're going with this. I don't worry too much about breaking existing solutions... the value of Exercism is really in the thinking/conversation around the solutions.

Yeah, death to db()! What do you think of the idea of asserting that insertion order is maintained for grade() — this would differentiate its output from that of allStudentsInAllGradesSorted()?

Other name options for allStudentsInAllGradesSorted():

• grades() (Rust track did that)
• studentsByGradeAlphabetical()
• allStudentsByGrade()
• roster()

[colin-mullikin]

@jtigger Sorry about the unnecessary snarkiness. I wasn't in the greatest of moods when I logged the issue, so it kind of bled into my typing.

I agree with your suggested fixes though. Maintaining insertion order unless the as-yet-unnamed aggregate sort function is called adds a bit of an extra obstacle, so that you can't just throw them all into a Map.

[jtigger]

Sorry about the unnecessary snarkiness. I wasn't in the greatest of moods when I logged the issue, so it kind of bled into my typing.

What... are you human or something?!?! ;)

[colin-mullikin]

Ssshhh, keep it down. I don't like to admit that. :P

[FridaTveit]

Maintaining assertion order for grade() sounds good and I like the name suggestions. Maybe studentsByGradeAlphabethical() or roster() would work best since the order of students is differnt to grade. I'm happy to make a PR for this, unless you wanted to @colin-mullikin? :)

[colin-mullikin]

@FridaTveit You can take it if you would like. I haven't even completed the exercise yet, so I would be happy to be your first guinea pig once you finish it. 👍

[FridaTveit]

Sure, will do! :)

[matthewmorgan]

@FridaTveit @jtigger I've participated in some other discussions on and offline about this exercise, and there was agreement around the thought that one of the valuable points of this exercise actually lives in the "bonus" section of README.md -- protecting the internal structure from mutation.

The sorting issue is some tracks and solutions was solved by sorting on insert, FYI, rather than having a method to trigger sorting on the DB or specifically to return a sorted list. Just a thought there.

Maybe outside of the scope of your proposed changes, but it would be nice to see a test for protection against mutation of the list object....again, just a thought as long as you're tinkering. :)

[davethomas11]

@matthewmorgan @FridaTveit

I've merged with @FridaTveit's most excellent update to the structure and new mutability tests! Putting a pull request up.

One change I opted to make is to remove the new test that asserts that insertion order is maintained. If this doesn't make sense please let me know, and I will make another commit to include that test.

My suggestion is that test is maybe something that should not be enforced. I myself was doing sorting on insertion and I've noticed that other users have chosen to do that too.

But it totally makes sense to me if that is something to enforce.

[FridaTveit]

The test for maintaining insertion order was something we added to make grade() more clearly different from studentsByGradeAlphabetical().get() (see earlier comments on this issue). It would also possibly make the exercise more interesting. But we can leave it out if people think that's best. @jtigger and @colin-mullikin, what do you think?

[matthewmorgan]

@colin-mullikin @davethomas11 @exercism/java do we feel like the PRs by @davethomas11, @colin-mullikin, and @FridaTveit have addressed this issue sufficiently?

[stkent]

+1 from me!

[stkent]

@colin-mullikin @davethomas11 @exercism/java poke! Has this issue been resolved to your satisfaction? Thanks :)

[matthewmorgan]

@stkent I'm feeling good about this one, FWIW.

[jtigger]

Worth a lot Mr. @matthewmorgan! ;)

:+1: from me.

[stkent]

Excellent; closing this!