dnd-character: Suggestions for improving constructor in stub and in reference solution

[sshine]

Mayur on Slack said that the DND Character exercise was hard to figure out.

I suspect that adding a constructor to the stub file would improve the situation.

As for the modifier method, shouldn't this be a static method?

And as for the reference solution, I would suggest the improvement to have Random dependency-injected into the constructor, since otherwise you can only do probabilistic testing, as is done now.

[lemoncurry]

Thank you @sshine for bringing this to our attention.

I will add the missing constructor to the stub file as soon as possible.

Regarding the modifier method, why would you declare this method static?

Also I am wondering if a reference solution with dependency injection of Random might be even more confusing for people who start out with learning Java (right now this is an exercise with level 2 difficulty). In addition I think the test resulting from the dependency injection would not truly test the functionality of the program. It would test the order of injection.
So I would suggest to keep the reference solution unchanged in that regard.

Maybe it would be warranted to increase the level of difficulty from 2 to 3 or 4.
What do you think @exercism/java ?

[sshine]

Regarding the modifier method, why would you declare this method static?

Because it depends on only its input, not on any object properties.

Since you don't need the context of an object to compute the function, the minimal implementation would be a static method.

Compare this to the Haskell solution, the C# solution and the Python solution where they're respectively a pure function independent of the Character type, a static method, and a top-level definition independent of the Character class. The closest of these to the Java track would be the C# one, I think.

[lemoncurry]

Agreed, thank you @sshine . I will add the static keyword to this method.

[sshine]

I am wondering if a reference solution with dependency injection of Random might be even more confusing for people who start out with learning Java (right now this is an exercise with level 2 difficulty).

OK, that is completely reasonable.

In addition I think the test resulting from the dependency injection would not truly test the functionality of the program. It would test the order of injection.

To be honest, I'm not very proficient with testing randomness via dependency injection; it was a topic I got involved with at work and have tried to find a good solution for. Otherwise I am only influenced by the principle of removing side-effects from functions to make them predictable, since this is the default in functional programming.

I suppose that if you mock Random, you still cannot be sure exactly how the implementation chooses to call and use random integers. So I'm not sure mocking Random can be used as a method to test anything but extreme cases like the XKCD strip. I suppose that one could test something like

for (int diceAlwaysRoll = 1; diceAlwaysRoll <= 6; diceAlwaysRoll++) {
    Random mockRandom = new DeterministicRandom(diceAlwaysRoll);
    DnDCharacter character = new DnDCharacter(mockRandom);
    assertEquals(character.getStrength(), 3 * diceAlwaysRoll);
    ...
}

although it wouldn't cover the logic of picking the least three. Having a deterministic, mock Random that cycles between four numbers wouldn't ensure that they align exactly with the number of nextInt() calls per character ability. I'd call it greybox testing at best.

The way the Java track tests that abilities are correct is the same way I've made the Haskell track do it, since property-based testing and mocking are, as far as I can see, orthogonal approaches. :-)

[FridaTveit]

@sshine @lemoncurry As for making the method static, we currently have a policy to prefer instance methods. Policies are of course always up for discussion and can be changed, but in that case I think that should be a separate issue 🙂

I agree that injecting Random could be confusing for users. And yes, it would probably be a good idea to increase the difficulty to 3 or 4 now that we have the test for randomness 🙂

[FridaTveit]

Also @sshine @lemoncurry why do you feel adding the constructor to the stub file would be an improvement? In general we only add enough code to the stub file so that you can run the tests without any compiler errors. As the constructor doesn't take any arguments, you don't need one for the tests to compile. And, correct me if I'm wrong as I haven't done this exercise, I don't think you necessarily need to implement a custom constructor to make all the tests pass. So we shouldn't necessarily be pushing users to a solution involving a custom constructor 🙂

[lemoncurry]

Thank you @FridaTveit for raising these important points.

Yes, it would not be necessary to add a stub for the constructor to pass the tests.
According to the policies it only says

provide stubs for all required constructors and methods.

So with required it is implied required to pass the tests without compiler error?
Then I misread the policy 😞 I should have checked the references also.

Regarding static methods, after reading the discussion that is referenced, I would agree to stay with this policy and stay with instance methods.

[FridaTveit]

Then I misread the policy

Sorry, it's probably not described very well. Feel free to update it to something that's more clear 🙂

[sshine]

@FridaTveit: Thank you for asking.

In general we only add enough code to the stub file so that you can run the tests without any compiler errors.

That seems reasonable, although I think it might also cause some students to not know where to start.

Another way would be to provide a track-specific hint that suggests how to proceed.

There could be yet other ways.

I'll elaborate on why I recommended the constructor in the stub:

---

So as far as I can tell, a mentor on the C track, Mayur, wrote the following on Slack:

So I'm currently doing Java track. I sometimes get stuck on the side exercises, for example I was stuck on DND character and protein translation.
On the website, there's no way to get help on any of this. Finding community solutions is only possible after making a submission.

This suggests to me that there's a gap in the way the exercise is understood.

To elaborate on my initial comment, "I suspect that adding a constructor to the stub file would improve the situation": This might indicate to the student that a starting point, and the meat of the exercise, is in the constructor.

So I think providing a constructor in the stub file would make the most important placeholder visible and move focus to the meat of the exercise, as opposed to figuring out where to place the die-rolling code.

Finding out where to begin is not something that has a high learning / time spent ratio.

You also wrote:

I don't think you necessarily need to implement a custom constructor to make all the tests pass

That's certainly true. modifier not depending on any object properties can be tested entirely without the use of a constructor; this becomes even more clear to the student once it's static.

we shouldn't necessarily be pushing users to a solution involving a custom constructor

I don't think you can solve the problem of randomly generating stats, save them and access them again without a custom constructor. So I suppose that the criterion of having the minimal stub that compiles the test may sometimes conflict with the didactic arguments that I present.

---

(The rest of this reply consists of personal anecdotes with minimal or non-existent stubs being the cause of problems. Feel free to disregard them.)

1. I've used the Perl track for training new recruits at my day job, and one thing we experienced was that the Perl track's choice of not providing a stub file for most of its exercises left our otherwise very qualified developers a little struck with where to begin. The argument on the Perl track, as I understood it, was two-fold: 1) The exercises we picked weren't core, so the student should have become accustomed to the structure of a Perl module when solving non-core exercises, and 2) if we were to provide stubs for all exercises on the Perl track, it'd be natural to start from the beginning. To address 1) we used the Exercism Teams to skip the queue, so there's no core track provided, and to address 2) I sent in some PRs for the exercises we focus on.

   On the Perl track, not providing the file still makes the test execute, but fail on the first test.

2. On the Ocaml track we started converting exercises to use the Base library. This went fine until we got to nucleotide-count. Ocaml features higher-order modules, and in some corners of the Base library, such as the basic key-value Map module (approximately an immutable hashmap, except it uses a balanced search tree internally), you need some quite particular syntax that isn't widely used in old Ocaml code, which means all older textbooks. It's in the beta version 2 of Real World Ocaml under "comparator witnesses".

   It took me several hours to figure out this syntax (working on the Ocaml track as a pretense to learn Ocaml), and unfortunately this didn't alarm me that the average student would encounter the same problem. Eventually someone chimed in and adding a single line to the stub:

   let empty = Map.empty (module Char)
   

   made the exercise infinitely easier by relieving the student of finding out how to convert the polymorphic Map.empty with the type signature

   # Map.empty ;;
   - : ('a, 'cmp) Map.comparator -> ('a, 'b, 'cmp) Map.t = <fun>
   

   to an empty char map. An explorer such as myself would first have tried to manually extract such a Map.comparator and stick it into Map.empty, but this is both labourious and not actually the way you ideally do it. But who'd know.

[FridaTveit]

If people are struggling with the exercise then we should definitely try to do something to help, so thanks for raising that @sshine 🙂 But I don't think we should be pushing users towards a certain solution. Upping the difficulty so that it more accurately reflects how hard the exercise is would be one improvement. I also like your idea of providing a track specific hint. We already do that for other exercises (e.g. https://github.com/exercism/java/blob/master/exercises/list-ops/.meta/hints.md) so I think that would be a good idea here as well 🙂

[sshine]

I don't think we should be pushing users towards a certain solution.

OK. I'm not sure I can imagine more than one solution besides the one where Random is injected into the constructor. The unit tests aren't compatible with this type of solution, and I can't imagine how it could be.

I believe I've left enough of an impression without contributing code to the solution, so I'll bid you a good day now. :-D