Piet Souris wrote:Junilu's description is clear as mud to me.
Piet Souris wrote:Where are the scores coming from, what are modifiers and what are these supposed to do?
Do we really need to test the rolling of the dice?
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.
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.
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
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: