Win a copy of Java Database Connections & Transactions (e-book only) this week in the JDBC forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Devaka Cooray
  • Knute Snortum
  • Paul Clapham
  • Tim Cooke
Sheriffs:
  • Liutauras Vilda
  • Jeanne Boyarsky
  • Bear Bibeault
Saloon Keepers:
  • Tim Moores
  • Stephan van Hulst
  • Ron McLeod
  • Piet Souris
  • Frits Walraven
Bartenders:
  • Ganesh Patekar
  • Tim Holloway
  • salvin francis

Exercism: Dungeons and Dragons Character  RSS feed

 
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
This is another problem from exercism.io where you're asked to write a DnDCharacter class who gets assigned random scores for six abilities: strength, wisdom, charisma, constitution, intelligence, and another one I can't remember right now. A score is calculated as the sum of the three largest rolls of four six-sided dice. There is also an ability() method which doesn't have a good specification. The unit test provided around that method simply asserts that its return value is in the range of 1..18 inclusive.

The requirements also stipulated that the scores for the six abilities should only be calculated once. There is a modifier() method that takes an int and calculates a value equal to the input + 10, then divide by 2 and then round down. So, if input is 3, then the modifier would be -4.

Have to go now, on the road, will finish later but think about this problem for now and how you would test it, particularly since scores are assigned randomly.
 
Saloon Keeper
Posts: 10251
216
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Knowing a bit of D&D, the sixth ability score is dexterity. Modifiers aren't (ability+10)/2 but (ability-10)/2.

I would test this by having the class accept a psuedo-random number generator, and then mock the PRNG so that I can control how the dice land:

 
Saloon Keeper
Posts: 3256
128
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hmmm...

I've never played D&D, and Junilu's description is clear as mud to me. What is there to test? Where are the scores coming from, what are modifiers and what are these supposed to do? Stephans suggestion looks impressive, but is pretty abstract and uses methods that I've never heard of. Do we really need to test the rolling of the dice? Or can we use our own Die class for this? Last remark: the sum of three dice should start at 3, not at 1.
 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Piet Souris wrote:Junilu's description is clear as mud to me.


And full of mistakes. Sorry, we were fetching my daughter from her school and I was posting from my phone just as my wife came back from browsing at the mall. Stephan has the correct details, and you're right, the test was checking scores were > 2 and < 19
 
Stephan van Hulst
Saloon Keeper
Posts: 10251
216
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Piet Souris wrote:Where are the scores coming from, what are modifiers and what are these supposed to do?


Ability scores are the basic numbers that determine what a character is good at. When a character is created, the player rolls dice in the way that Junilu explained to get six scores, and then the player can assign those scores to the six abilities. In this case however, the scores are apparently assigned randomly upon character creation.

Modifiers are derived from ability scores. A score of 10 indicates "average" ability and so leads to no modifiers. A score of 18 indicates super-human ability, and leads to a modifier of +4 on dice rolls for actions that are associated with the ability. For instance, attacking a monster with a melee weapon is associated with the Strength ability, and fighters with a Strength of 18 can add 4 to dice rolls that determine whether their attack is successful, and they can add 4 to dice rolls that determine how much damage they do. Dexterity is associated with ranged attacks and dodging, Constitution determines how much hit points a character gets when they level up, Intelligence and Wisdom are associated with spell-casting and a whole bunch of other actions, and Charisma determines how convincing the character is in speech.

I guess for more explanation we'll have to wait for Junilu to finish.

Do we really need to test the rolling of the dice?


My code doesn't test the rolling of the dice. It mocks rolling of dice to test whether a characters is generated according to the rules.
 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sorry for the bad regurgitation of the requirements. Stephan got most of the details.

Six abilities: strength, dexterity, constitution, intelligence, wisdom and charisma

Each ability score is determined by rolling four 6-sided dice, discarding the lowest number, and summing up the other three. As Piet pointed out, the range of scores is [3-18] (inclusive)

Initial hitpoints is 10 + the character's constitution modifier.

An ability modifier is calculated by subtracting 10 from the ability score, then dividing by 2, then rounding down. So, if a character's constitution score is 3, then its constitution modifier is (3 - 10) / 2, then round down, giving -4.

That's all that was given in the text requirements. The unit test provided, however, had this:

Based on the name of the test, I think the intent for the ability() method was to return the score of a randomly selected ability. I think this test falls short though because the test name does not really fit well with what the test code is actually doing. To me, this is an excellent example of how a poorly written test leads to poorly understood or misunderstood requirements, which in turn can lead to bugs.

Edit: I take that back. It's not that the test name doesn't fit well with what the test code is doing, because in fact it kind of does. I just feel there's still something missing with this test as I've seen some implementations that suggest the intent of the ability() method can be misunderstood.  I think testing with a mock PRNG would allow you to demonstrate that the ability() method works correctly for all possible abilities it can pick out. As it is now, the test would just randomly pick one ability per test run. That kind of leaves me wondering if it would in fact be able to correctly pick out any of the six abilities.
 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Stephan,

The (holy) cow(!   ) is for this:

The elegance is stunning.
 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The unit tests implied that the DnDCharacter class would only have a no-args constructor. This is what I can up with:

This seems very clunky compared to what you wrote, Stephan.
 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Here's what I have after incorporating what I learned from Stephan and his implementation:

I have to go to bed now; my son is graduating (from college! ) tomorrow and I need to get an early start. There is so much I'd like to discuss about Stephan's code but that will have to wait until Monday evening, US time.

Thanks again, Stephan, for sharing your stream-jutsu. It's pretty awe-inspiring, man.    
 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Darn it, I just realized what was bugging me about my implementation of ability(): it's just a happy coincidence that there are six abilities and that the dice I'm using has six sides. So, while the mechanics work, the semantics are nonsensical.

I should have PIEd this (Program Intent-fully and Expressively)

or maybe

where
 
Open to suggestions for names besides any() and randomAbility(). random()?

But then again, how do I test this kind of code?

Really need to hit the sack now but I look forward to discussing this some more when I can get back to you guys.
 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It just occurred to me that there is no easy way to test code like this:

... or is there? Hmmm... I just got an idea. Will follow up later.
 
Stephan van Hulst
Saloon Keeper
Posts: 10251
216
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Why not? Like with any method who's behavior depends on a service (your Dice), you mock the service and then test whether the method returns the expected value.

Let the dice return a known sequence of rolls and then test if the method returns the sum of the highest three.
 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Here's the refactored code that is more testable:

and here are some tests for that:

My mindset is to protect myself from screwing up anything in the code by causing a test to fail when any regression is introduced. I think the only thing not covered here is the expression "times - limit" in the sumOfLargest() method.
 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Stephan van Hulst wrote:Let the dice return a known sequence of rolls and then test if the method returns the sum of the highest three.


Exactly  

I'd love to see a different way of approaching the tests than what I did.
 
Stephan van Hulst
Saloon Keeper
Posts: 10251
216
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Congratulations on your son's graduation!

There are a few criticisms I have about your code. First, it doesn't make sense to me that dice is an instance field and that sumOfLargest() is an instance method. You don't need the dice for anything other than instantiation. It's also a pity that you let the class create its own dice rather than having dice injected in the constructor. It would have been much easier to test your class because then you can create it with a mock instance of Dice. The same with generator in your Dice class. Random is a service that more often than not you want to inject, and not create inside the class that depends on it.

I'm guessing the modifier() method is an instance method because that's what the exercise demands. Bad form though. It doesn't depend on instance fields and so it can be static (and private).

I don't like that you made the sumOfLargest() method package private to facilitate testing. Private methods are like an implementation detail. Test the methods that depend on it instead. If the private method is complex and you want to be sure that it works properly, there are testing frameworks that access private members.

What stories do the phrases warrior.sumOfLargest() or mage.sumWithoutSmallest() tell me? Don't forget your problem domain.

Why are the number of sides of dice constant? Had you passed the number of sides in the constructor, you could have created a separate die for your ability() method that has the same number of sides as the amount of ability scores there are. Or you could get rid of the Dice class altogether, because it's really just a paper thin wrapper around a Random, without really adding much extra behavior to it.

If you decide to keep the Dice class, I wonder why the roll() method takes a times parameter. Why not just let the client call the limit() method? A call would then look like: die.rollRepeatedly().limit(4).

Maybe a good approach is to move the methods to a separate utility class that is responsible for generating ability scores. It then has a clear responsibility that is decoupled from the DnDCharacter class:

 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
All good thoughts/questions, Stephan.

There are 9K plus students in The Horseshoe each receiving their own diplomas today so it's going to be a while before I can address them all.

All in all, Ohio State University is graduating 12,213 students this spring!
 
Junilu Lacar
Sheriff
Posts: 13480
222
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Stephan van Hulst wrote:it doesn't make sense to me that dice is an instance field and that sumOfLargest() is an instance method. You don't need the dice for anything other than instantiation.


Agreed. These should be static

It's also a pity that you let the class create its own dice rather than having dice injected in the constructor. It would have been much easier to test your class because then you can create it with a mock instance of Dice. The same with generator in your Dice class. Random is a service that more often than not you want to inject, and not create inside the class that depends on it.


Agreed. One must always balance simplicity with testability. Sometimes the cost of testability is more complexity but I think we should err on the side of testability eventually.

I'm guessing the modifier() method is an instance method because that's what the exercise demands.  Bad form though. It doesn't depend on instance fields and so it can be static (and private).


The unit tests and the skeleton code that the exercise started out with did have modifier() defined that way, so I just went with it. I wouldn't think those are hard requirements though so your point that it should be static is valid. Not sure about making it private though. The tests hint that some outside code might want to call it. Need more context about its use.

I don't like that you made the sumOfLargest() method package private to facilitate testing. Private methods are like an implementation detail. Test the methods that depend on it instead. If the private method is complex and you want to be sure that it works properly, there are testing frameworks that access private members.

What stories do the phrases warrior.sumOfLargest() or mage.sumWithoutSmallest() tell me? Don't forget your problem domain.


Yeah, it feels a little icky. Still thinking about this method and how to deal with it.

Why are the number of sides of dice constant? Had you passed the number of sides in the constructor, you could have created a separate die for your ability() method that has the same number of sides as the amount of ability scores there are. Or you could get rid of the Dice class altogether, because it's really just a paper thin wrapper around a Random, without really adding much extra behavior to it.


Point is valid and again goes back to the need to balance simplicity with testability, and (eventually) erring on the side of testability. I think the code will eventually evolve in that direction. It's a good exercise to be able to feel the forces that compel you to move in that direction though and this exercise is great practice. Of course, you also need great feedback like what you've been giving.


If you decide to keep the Dice class, I wonder why the roll() method takes a times parameter. Why not just let the client call the limit() method? A call would then look like: die.rollRepeatedly().limit(4).

Maybe a good approach is to move the methods to a separate utility class that is responsible for generating ability scores. It then has a clear responsibility that is decoupled from the DnDCharacter class


Again, good points. I will think this over and continue refactoring.

I'm going to be on the road again this week so my replies may be fewer and farther apart but I'll keep at this in the evenings when I can.

Thanks again for the discussion and great feedback.

I'd encourage you all to go to exercism.io and start working through these challenges as well. Unless it just happens to be another ksnortum who programs in Java out there, I'm pretty sure our Knute is also working on this.

Happy Monday everyone!
 
Master Rancher
Posts: 4087
47
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Written a few of these in my time.
This made me smile.

Damn sight neater than the usual generator code.

Also this:

Stephan van Hulst wrote:Maybe a good approach is to move the methods to a separate utility class that is responsible for generating ability scores. It then has a clear responsibility that is decoupled from the DnDCharacter class:



Yes.
Yes, yes yes.

At the end you want a character that simply says what the character is and what they can do.
You don't really want the bunch of generators as a part of that.

The advantage of that is you can define the character early on, and add the generators in as you go (there's more than just rolling the stats).
The generator set up is pretty much an entirely separate module.

Obviously this problem is only handling a very small section of the larger problem, but it's a good idea to keep the two things (the resultant Character, and the CharacterGenerator) apart.
 
Sheriff
Posts: 6789
469
BSD Linux Mac OS X VI Editor
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
When not much free-time left, sounds like a good plan to go to time's debt. Will have to check this exercism.io.

For now just registering to watch this thread to get updates from your talks.
 
Stephan van Hulst
Saloon Keeper
Posts: 10251
216
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Any updates Junilu?
 
And inside of my fortune cookie was this tiny ad:
how do I do my own kindle-like thing - without amazon
https://coderanch.com/t/711421/engineering/kindle-amazon
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!