DnD character exercise - tests do not entirely encapsulate the requirements

[ChrisCookOC]

Just noticed while running through DnD character that if you follow it strictly test-driven (bare minimum-functionality to get the tests passing), you can skip the whole random-generation aspect of the test and just hardcode the values of ability, strength etc. See iteration 1 here for example: https://exercism.org/tracks/java/exercises/dnd-character/solutions/ChrisCookOC

Perhaps could be improved by - in testRandomCharacterIsValid - keeping a copy of the first character and making sure that at least one of the 1000 other generated characters is different. Without actually trying to do the calculations, the chance of generating 1000 identical characters should be sufficiently zero to not be concerned about false negatives.

[andrerfcsantos]

Thanks for reporting this!

This is indeed an issue with the lack of tests.

keeping a copy of the first character and making sure that at least one of the 1000 other generated characters is different. Without actually trying to do the calculations, the chance of generating 1000 identical characters should be sufficiently zero to not be concerned about false negatives.

I agree with this solution, anyone is welcome to submit a PR for it! However, I don't think we need 1000 iterations, maybe 100, 500 max is enough?

[umuser]

I would like to fix this, but I found another problem with the tests. Calculating abilities does not test that 3 largest numbers are used as the exercise description says

...You do this by rolling four 6-sided dice and record the sum of the largest three dice

For example, some solutions contain code like this new Random().ints(4, 1, 7).sorted().limit(3).sum();, but .sorted() sorts in ascending order so the sum of the smallest numbers is returned.

Maybe a solution could be that throwing dice is refactored into its own function that returns a list of 4 numbers. Also, the ability function should then be changed to take a list of 4 numbers as input. This way ability calculation can be tested with certain input and expected results.

List<Integer> throwDice() {..}

int ability(List<Integer> diceValues) {..}

@Test
public void testAbilityCalculation() {
    assertEquals(9, dndCharacter.ability(List.of(1,2,3,4)));
}

[ChrisCookOC]

Re: 1000 - that suggests mainly comes from the fact it already does an iteration of 1000

@umuser - that's a good spot too! Certainly would be happy with ability taking in a List<Integer> as a solution to that, although I wouldn't necessarily mandate that you use a throwDice function defined as such? It's probably the nicest way to do it, but since we won't be testing the throwDice method directly we should just let it be defined privately however people want?

[andrerfcsantos]

@umuser Feel free to submit a PR fixing that problem too.

I think it's reasonable to have ability take a list as an argument, and I'm also ok with having a throwDice function defined like that. However, if adding such function, we should add tests for that function too.

[andrerfcsantos]

PR https://github.com/exercism/java/pull/2218 has been merged.