typescript icon indicating copy to clipboard operation
typescript copied to clipboard

Grade School Exercise - Map Implicitly Initialized Incorrectly

Open arcsector opened this issue 4 years ago • 5 comments

Problem Summary

The test for Exercise 10 (Grade School) implicitly defines all test Objects as Map<string, string[]> which is contrary to how it is initialized (this is from line 25):

const expectedDb = new Map(Object.entries({ 2: ['Aimee'] }))

Solutions

Switch tests to correct explicit definition

Explicitly define expectedDb to be Map<number, string[]>:

const expectedDb: Map<number, string[]> = new Map().set(2, ['Aimee']);

OR

const expectedDb: Map<number, string[]> = new Map([ [2, ['Aimee']] ]);

The reasoning for this is that when you initialize using a generic Map(Object.entries( {} )), but use a raw number type to insert in the dictionary { 2: ['Aimee'] }, this is confusing for the user, and doesn't provide the information that we should be using type Map<string, string[]>

Switch tests to correct explicit initialization

Explicitly define expectedDb to be Map<string, string[]>:

const expectedDb: Map<string, string[]> = new Map(Object.entries({ "2": ['Aimee'] }))

Note the two changes here are the explicit type definition as well as the stringify-ing of the number Map key.

arcsector avatar Apr 10 '21 22:04 arcsector

Hi there!

5 hours ago I pushed an update to this exercise. Due to #387 it won't sync to the website just yet, but can you check if these tests and this proof of implementation would solve your issues?

https://github.com/exercism/typescript/blob/main/exercises/practice/grade-school/grade-school.test.ts

https://github.com/exercism/typescript/blob/main/exercises/practice/grade-school/.meta/proof.ci.ts

SleeplessByte avatar Apr 10 '21 23:04 SleeplessByte

@SleeplessByte Not particularly:

  1. The method names are not the same as the previous version of the exercise
  2. The test roster cannot be modified outside of module needs to use roster.get() in order to take care of keys not found 2a. The test roster cannot be modified outside of module needs to have a result exception due to a possible undefined return from roster.get()
  3. The default datatype that everything is stored in is a plain dictionary as opposed to a Map

arcsector avatar Apr 11 '21 00:04 arcsector

It's intentionally no longer a Map. You can use a Map internally if you like, but the test suite doesn't impose it.

The method names have also intentionally be renamed to match the problem-specifications.

The test roster cannot be modified outside of module needs to have a result exception due to a possible undefined return from roster.get()

These exercises are usually not covering all edge cases. If you think that these should be covered, we do take PRs. Often there is a reason why we have included / have not included a test case, but I think for this particular one there is none 👍🏽

SleeplessByte avatar Apr 11 '21 00:04 SleeplessByte

Understood; I think persisting the current format of the datatypes as much as possible would be preferred to most users, especially those who have submitted public solutions, but other than that, yes this would satisfy this issue, since the default dictionary keys are type number and not implicitly interpreted as string.

I think we can close this if this is the way you're intending to go.

arcsector avatar Apr 11 '21 00:04 arcsector

Eh. I'm in the middle. I totally see the value in persisting the data types as is. I do really like to keep these new methods names (because of the canonical dataset), but if you see a way to bring back Map, I'd take that PR!

SleeplessByte avatar Apr 11 '21 02:04 SleeplessByte