Add dnd-character exercise

[ErikSchierboom]

No description provided.

[tasxatzial]

It's very difficult to review this. It includes more tests than the canonical data, the descriptions don't seem to match the canonical data, and there are even tests like (>= (count (set (repeatedly 100 #(dnd-character/ability)))) 5), which I can't locate in the canonical data.

Extensive deviations from the canonical data make it very challenging to perform manual comparisons when something needs to be inspected.

Were there specific reasons for these deviations?

[ErikSchierboom]

Were there specific reasons for these deviations?

Yes, because this an exercise which canonical data in large part describe "properties," not concrete test values. This is due to the fact that we're dealing with randomness, which doesn't fit well into the regular canonical data format.

It's very difficult to review this. It includes more tests than the canonical data, the descriptions don't seem to match the canonical data, and there are even tests like (>= (count (set (repeatedly 100 #(dnd-character/ability)))) 5), which I can't locate in the canonical data.
Extensive deviations from the canonical data make it very challenging to perform manual comparisons when something needs to be inspected.

I understand, but in this case I feel extra tests are warranted.
Take this test case, which is the only test case for the ability function:

{
      "uuid": "4f28f19c-2e47-4453-a46a-c0d365259c14",
      "description": "random ability is within range",
      "scenarios": ["random"],
      "property": "ability",
      "input": {},
      "expected": "score >= 3 && score <= 18"
    },

In essence, this is specifying two things:

1. The resulting value should be between 3 and 18
2. Each value is generated randomly

This is why the additional test is added. We could combine them into a single test, but I feel that a separate test makes more sense. Basically what I want to test is that the function does not keep on returning the same values, but that it is doing something random.

A similar argument applies to the tests for the character property:

{
      "uuid": "385d7e72-864f-4e88-8279-81a7d75b04ad",
      "description": "random character is valid",
      "scenarios": ["random"],
      "property": "character",
      "input": {},
      "expected": {
        "strength": "strength >= 3 && strength <= 18",
        "dexterity": "dexterity >= 3 && dexterity <= 18",
        "constitution": "constitution >= 3 && constitution <= 18",
        "intelligence": "intelligence >= 3 && intelligence <= 18",
        "wisdom": "wisdom >= 3 && wisdom <= 18",
        "charisma": "charisma >= 3 && charisma <= 18",
        "hitpoints": "hitpoints == 10 + modifier(constitution)"
      }
    }

As you can see, this test case actually has at least 7 different tests, and with the added test for random generation, we get 8 tests encoded by a single test case property.

[ErikSchierboom]

@tasxatzial What do you think about naming the functions rand-ability and rand-character to be in line with what is in clojure.core?

[tasxatzial]

My preference is to use verbs for function names, but I understand this can sometimes result in verbose names. As such, I'm not too particular about naming. I don't believe the official documentation strictly follows this guideline either.

In my view, reasons to switch from ability to rand-ability would be:

• It's a bit more specific.
• It's longer, which makes it less likely for people to reuse it as a variable inside the function. By switching, it becomes possible to use a variable named ability and return it as the final step.

[ErikSchierboom]

Okay. I've updated the PR

[tasxatzial]

Couple extra suggestions before i do a proper review:

• Let's rename the function from modifier to score-modifier
• Template file needs to be updated with new changes:
  • modifier -> score-modifier
  • character -> rand-character
  • ability -> rand-ability
• Separate parent/child descriptions with >> (i prefer the ▶) instead of -

[ErikSchierboom]

Separate parent/child descriptions with >> (i prefer the ▶) instead of -

Is this something that you would like to see changed for all exercises?

[ErikSchierboom]

I've updated

[ErikSchierboom]

@tasxatzial I've created a PR to update the separator: #779 Let's get that merged first and then I'll update this PR

[tasxatzial]

Ok, there are still some updates pending on this one:

• New description separator
• Template needs to get the new function names

One case seems to have been excluded. It would be helpful to mention why. I'm considering creating an issue that lists all the factors considered for each exercise. This will make it easier to track important details for future reference.

[tasxatzial]

With the recent updates, I'll wait until all open PRs are updated.

[ErikSchierboom]

Okay updated. I've tried to include the "calculated only once" tests. Let me know what you think. The idea is that the value is calculated each time the key is accessed, but is that even possible in Clojure?

[tasxatzial]

OK, i've finally managed to go over this. It's not as complicated as i though it would be. Hopefully i haven't missed anything.

The modifier tests look fine.

The ability tests are broken into two, as you said. The second test looks fine. But the first one is generating the ability only once. Is this sufficient for checking that values are within range?

The 'random character test is valid' tests are split into multiple. I'm guessing you went with the property-tested suggestion from the specs? Same as the ability tests, there's one test for each ability. Is this sufficient for checking that values are within range?

Regarding the 'each ability is only calculated once' tests. What's the purpose of those tests in the specs? I'm not even sure how to interpret them.

[ErikSchierboom]

the first one is generating the ability only once. Is this sufficient for checking that values are within range?

I guess it's better to also do this for a number of times. Good call.

The 'random character test is valid' tests are split into multiple. I'm guessing you went with the property-tested suggestion from the specs?

Correct. If you'd like

Same as the ability tests, there's one test for each ability. Is this sufficient for checking that values are within range?

We should also do this check repeatedly, certainly!

Regarding the 'each ability is only calculated once' tests. What's the purpose of those tests in the specs? I'm not even sure how to interpret them.

The original idea of the canonical data is that there will be languages where the character's abilities are methods. The test case is to check that that method isn't just returning a random value each time it is called. I guess this doesn't make sense in Clojure, does it?

[tasxatzial]

The original idea of the canonical data is that there will be languages where the character's abilities are methods. The test case is to check that that method isn't just returning a random value each time it is called. I guess this doesn't make sense in Clojure, does it?

We can make the character's abilities function calls if we want, but i believe that something like (:strength character) isn't going to calculate the ability. I guess we can remove the test for now and monitor the solutions to see if we are wrong.

[ErikSchierboom]

Ready for a new check

[tasxatzial]

Looks good!