Problem
For my game I need to roll some values with a die. A die is formally described as:
nDs[+a]
- n amount of die (optional, 1 of not set, zero must be set explicitly)
Dis a fixed part representing this expression as die- s amount sides of the die/dice
- +a additional amount
examples:
2D6: 2 six-sided dice, result 2..122D6+1: 2 six-sided dice + 1, result 3..133D4+2: 3 four-sided dice + 2, result 5..14
Implementation of class Die
public class Die {
private static final String INPUT_VALIDATION_PATTERN = "^\d*D\d*(\+\d+)?$";
private static final String SINGLE_DIE_PATTERN = "\d*D\d*";
private static final String ADDITION_SEPARATOR_PATTERN = "\+";
private static final String DIE_SEPARATOR_PATTERN = "D";
private static final String ADD_PATTERN = "\d*";
private static final String WHITESPACE_PATTERN = "\s+";
//package visible for testing
final int amount;
final int sides;
final int addition;
public Die(String input) {
String adjusted = input.toUpperCase(Locale.ROOT).replaceAll(WHITESPACE_PATTERN,"");
if (!adjusted.matches(INPUT_VALIDATION_PATTERN)) {
throw new IllegalArgumentException(
"invalid dice input pattern: "+adjusted+" - does not match "+INPUT_VALIDATION_PATTERN);
}
String[] parts = input.split(ADDITION_SEPARATOR_PATTERN);
if(parts[0].matches(SINGLE_DIE_PATTERN)){
String dicePart = parts[0];
int dIndex = dicePart.indexOf(DIE_SEPARATOR_PATTERN);
if(dIndex == 0){
amount = 1;
}else{
amount = Integer.parseInt(dicePart.substring(0,dIndex));
}
sides = Integer.parseInt(dicePart.substring(dIndex+1));
}else{
amount = 0;
sides = 0;
}
if(parts[0].matches(ADD_PATTERN)){
addition = Integer.parseInt(parts[0]);
}else if(parts.length == 2 && parts[1].matches(ADD_PATTERN)){
addition = Integer.parseInt(parts[1]);
}else{
addition = 0;
}
}
public int result(long seed){
Random random = new Random(seed);
int result = 0;
for(int roll = 0; roll < amount; roll++){
result = result + random.nextInt(sides)+1;
}
result = result + addition;
return result;
}
public int result(){
return result(0);
}
public int min(){
return amount + addition;
}
public int max(){
return amount * sides + addition;
}
}
Test Case DieTest
public class DieTest {
@Test
public void singleDieTest(){
Die d6 = new Die("D6");
Assert.assertEquals(6, d6.sides);
Assert.assertEquals(1, d6.amount);
Assert.assertEquals(0,d6.addition);
Assert.assertEquals(1,d6.min());
Assert.assertEquals(6,d6.max());
Die oneD6 = new Die("1D6");
Assert.assertEquals(6, oneD6.sides);
Assert.assertEquals(1, oneD6.amount);
Assert.assertEquals(0,oneD6.addition);
Assert.assertEquals(1,oneD6.min());
Assert.assertEquals(6,oneD6.max());
Die twoD6 = new Die("2D6");
Assert.assertEquals(6, twoD6.sides);
Assert.assertEquals(2, twoD6.amount);
Assert.assertEquals(0,twoD6.addition);
Assert.assertEquals(2,twoD6.min());
Assert.assertEquals(12,twoD6.max());
Die twentyD6 = new Die("20D6");
Assert.assertEquals(6, twentyD6.sides);
Assert.assertEquals(20, twentyD6.amount);
Assert.assertEquals(0,twentyD6.addition);
Assert.assertEquals(20,twentyD6.min());
Assert.assertEquals(120,twentyD6.max());
Die twentyD60 = new Die("20D60");
Assert.assertEquals(60, twentyD60.sides);
Assert.assertEquals(20, twentyD60.amount);
Assert.assertEquals(0,twentyD60.addition);
Assert.assertEquals(20,twentyD60.min());
Assert.assertEquals(1200,twentyD60.max());
}
@Test
public void dieWithSingleAdditionTest(){
Die d6 = new Die("D6+1");
Assert.assertEquals(6, d6.sides);
Assert.assertEquals(1, d6.amount);
Assert.assertEquals(1,d6.addition);
Assert.assertEquals(2,d6.min());
Assert.assertEquals(7,d6.max());
Die oneD6 = new Die("1D6+1");
Assert.assertEquals(6, oneD6.sides);
Assert.assertEquals(1, oneD6.amount);
Assert.assertEquals(1,oneD6.addition);
Assert.assertEquals(2,oneD6.min());
Assert.assertEquals(7,oneD6.max());
Die twoD6 = new Die("2D6+1");
Assert.assertEquals(6, twoD6.sides);
Assert.assertEquals(2, twoD6.amount);
Assert.assertEquals(1,twoD6.addition);
Assert.assertEquals(3,twoD6.min());
Assert.assertEquals(13,twoD6.max());
Die twentyD6 = new Die("20D6+1");
Assert.assertEquals(6, twentyD6.sides);
Assert.assertEquals(20, twentyD6.amount);
Assert.assertEquals(1,twentyD6.addition);
Assert.assertEquals(21,twentyD6.min());
Assert.assertEquals(121,twentyD6.max());
Die twentyD60 = new Die("20D60+1");
Assert.assertEquals(60, twentyD60.sides);
Assert.assertEquals(20, twentyD60.amount);
Assert.assertEquals(1,twentyD60.addition);
Assert.assertEquals(21,twentyD60.min());
Assert.assertEquals(1201,twentyD60.max());
}
@Test
public void dieWithTwoDigitAdditionTest(){
Die d6 = new Die("D6+13");
Assert.assertEquals(6, d6.sides);
Assert.assertEquals(1, d6.amount);
Assert.assertEquals(13,d6.addition);
Assert.assertEquals(14,d6.min());
Assert.assertEquals(19,d6.max());
Die oneD6 = new Die("1D6+13");
Assert.assertEquals(6, oneD6.sides);
Assert.assertEquals(1, oneD6.amount);
Assert.assertEquals(13,oneD6.addition);
Assert.assertEquals(14,oneD6.min());
Assert.assertEquals(19,oneD6.max());
Die twoD6 = new Die("2D6+13");
Assert.assertEquals(6, twoD6.sides);
Assert.assertEquals(2, twoD6.amount);
Assert.assertEquals(13,twoD6.addition);
Assert.assertEquals(15,twoD6.min());
Assert.assertEquals(25,twoD6.max());
Die twentyD6 = new Die("20D6+13");
Assert.assertEquals(6, twentyD6.sides);
Assert.assertEquals(20, twentyD6.amount);
Assert.assertEquals(13,twentyD6.addition);
Assert.assertEquals(33,twentyD6.min());
Assert.assertEquals(133,twentyD6.max());
Die twentyD60 = new Die("20D60+13");
Assert.assertEquals(60, twentyD60.sides);
Assert.assertEquals(20, twentyD60.amount);
Assert.assertEquals(13,twentyD60.addition);
Assert.assertEquals(33,twentyD60.min());
Assert.assertEquals(1213,twentyD60.max());
}
@Test
public void rangeTest(){
Die twoD6plus1 = new Die("2D6+1");
for (int i = 0; i < 100; i++){
Assert.assertTrue(twoD6plus1.result() >= twoD6plus1.min() );
Assert.assertTrue(twoD6plus1.result() <= twoD6plus1.max() );
}
}
}
Solution
Bug in the validation pattern
The INPUT_VALIDATION_PATTERN doesn’t work correctly, as it matches the invalid input "D". This is due to the pattern looking for any number of digits (*) after the D. Changing the pattern to look for at least one occurrence (+) seems to fix this.
Simple solution using capture groups
After some fiddling around with your pattern I found that regex can be used to solve this rather nicely, using ^(d+)?(Dd+)(+d+)?$.
String pattern = "^(\d+)?(D\d+)(\+\d+)?$";
Pattern r = Pattern.compile(pattern);
Matcher m = r.matcher(in);
if (m.find()) {
...
}
If the input matches, m.find() returns true. You then use m.group(x) to access the capture results, with m.group(1) being the number of dice, m.group(2) being the number of sides and m.group(3) being the addition. If there is no number for multiple dice/addition, then the groups 1/3 return null, which can easily be tested for. Do note that the strings in group 2 and 3 start with D and + respectively due to how the groups are set up. How the full constructor would look is left as an exercise for the reader 🙂
Explaination:
^(d+)?(Dd+)(+d+)?$
^ $ // start/end of string
( ) // capture group 1: capture
? // one or zero of
d+ // at least one digit
( ) // capture group 2: capture
Dd+ // a "D" followed by at least one digit
( ) // capture group 3: capture
? // one or zero of
+d+ // a "+" followed by at least one digit
I’m no expert on regex, so please test this thoroughly. I’ve thrown your tests, some edge cases and random things at this regex and it worked, but you can’t be too careful.
We don’t know how your Die fits into the game as a whole, but the way you’re rolling your results seems counter-intuitive to me. I’d expect to be able to call result and get a unique value each time. However, if I roll 5D6+2, I’ll always get the same answer (22 on my system), because it’s always creating a new Random with a known seed (0). Consider moving Random outside of the result method, or having a different value for the default seed (such as something time-based), or separating the rolling responsibility out into a different class entirely.
public int result(long seed){
Random random = new Random(seed);
int result = 0;
for(int roll = 0; roll < amount; roll++){
result = result + random.nextInt(sides)+1;
}
result = result + addition;
return result;
}
public int result(){
return result(0);
}
As a side note… should your class be called Dice? It seems to represent more than one Die? amount could then perhaps be numberOfDie..
You can use named groups in the pattern, as well, which will make the code less brittle to pattern changes. (Example: If you want to add in the feature of multiplication in the future then you could have, e.g., 3d6x2+1 meaning that the addition part would be the 4th matching group instead of the 3rd, and if you refer to groups by order you would have to update the addition part of your code. If you refer to groups by name, then the addition group is still the addition group regardless of where it moves to, so you would only have to touch the new code.) It can also make counting groups in general a lot easier.
String diceNotation = "(?<amount>\d+)?D(?<faces>\d+)(\+(?<addition>\d))?";
Pattern dicePattern = Pattern.compile(diceNotation, Pattern.CASE_INSENSITIVE);
Matcher diceMatcher = dicePattern.matcher(input);
int amount = diceMatcher.group("amount") != null ?
Integer.parseInt(diceMatcher.group("amount")) :
1;
int faces = Integer.parseInt(diceMatcher.group("faces"));
int add = diceMatcher.group("add") != null ?
Integer.parseInt(diceMatcher.group("add")) :
0;
Note that Matcher.group is returning null when a pattern doesn’t match at all. There are some subtleties of regex here, such as (?<foo>a*) will match an empty string and return an empty string as the foo group, but (?<foo>a+)? will match an empty string with the whole pattern but return null as the foo group.
Also, I have taken the liberty of including case_insensitive, because 3d6 is just as good as 3D6. You could also liberally sprinkle s* between each group in order to allow arbitrary white space, so that 3 d 6 = 3d 6 = 3d6 = etc...
As others have mentioned, there is a bug in the pattern – see their answers for assistance on fixing it 😉
Apart from this, I’d like to remind you to refactor your tests as well. The repeated pattern in the tests, along with the relatively large amount of boilerplate, make it hard to distinguish if there are any cases you have missed or if there are duplicates.
There are solutions out there for helping to write tests that focus on what you are actually trying to test, instead of writing pages full of asserts. For instance, https://www.baeldung.com/parameterized-tests-junit-5 :
private static Stream<Arguments> shouldParseDice() {
return Stream.of(
Arguments.of("D6", 1, 6, 0, 1, 6),
Arguments.of("1D6", 1, 6, 0, 1, 6),
Arguments.of("2D6", 2, 6, 0, 2, 12),
Arguments.of("2D6+1", 2, 6, 1, 3, 13),
// ...
Arguments.of("30D11+40", 30, 11, 40, 70, 370)
);
}
@ParameterizedTest
@MethodSource
void shouldParseDice(
String input,
Integer amount,
Integer sides,
Integer addition,
Integer min,
Integer max) {
Die die = new Die(input);
Assertions.assertEquals(amount, die.amount);
Assertions.assertEquals(sides, die.sides);
Assertions.assertEquals(addition, die.addition);
Assertions.assertEquals(min, die.min());
Assertions.assertEquals(max, die.max());
}
private static Stream<String> shouldFailDieParsing() {
return Stream.of("D", "G", "2D6-4", "", null, "-23D-2", "-2D1");
}
@ParameterizedTest
@MethodSource
void shouldFailDieParsing(String input) {
Assertions.assertThrows<IllegalArgumentException>(() => new Die(input));
}
If, for whatever reason, you are unable to make the move to junit5, you could simply roll your own in junit4, though it is slightly less tool supported and need some more explicit details to be able to easily understand the output when it fails:
@Test
public void shouldParseDice() {
validateDieParsing("D6", 1, 6, 0, 1, 6);
validateDieParsing("1D6", 1, 6, 0, 1, 6);
validateDieParsing("2D6", 2, 6, 0, 2, 12);
validateDieParsing("2D6+1", 2, 6, 1, 3, 13);
// ...
validateDieParsing("30D11+40", 30, 11, 40, 70, 370);
}
private void validateDieParsing(
String input,
Integer amount,
Integer sides,
Integer addition,
Integer min,
Integer max) {
Die die = new Die(input);
String message = "input was: " + input;
Assert.assertEquals(message, amount, die.amount);
Assert.assertEquals(message, sides, die.sides);
Assert.assertEquals(message, addition, die.addition);
Assert.assertEquals(message, min, die.min());
Assert.assertEquals(message, max, die.max());
}
@Test
public void shouldFailDieParsing() {
return Stream.of("D", "G", "2D6-4", "", null, "-23D-2", "-2D1")
.forEach(it -> shouldFailDieParsing(it));
}
private void shouldFailDieParsing(String input) {
try {
new Die(input);
fail("did not throw exception on expected bad input = " + input);
} catch (IllegalArgumentException e) {
// expected, ignore
} catch (Exception e) {
e.printStackTrace():
fail("Caught unexpected exception for input = " + input);
}
}
Transforming your tests to a format where you can easily see which cases you have covered, makes it easier to write good tests. In this case, you may have discovered the “D” input giving you trouble early on.
From the design standpoint it seems you are mixing two things – abstraction of a die (that has certain properties like number of faces) and has some behavior associated with it (as it can be rolled for certain number to be a result) AND parsing (even though you call it differently see: https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) seemingly arbitrary string.
I don’t know application of this class – which, imho, should be major driver the design of a class, but you might consider separating those two – especially if creating dice set from a raw string is somehow accidental (so if you ever have a need to roll dice but can do it without parsing strings – my guess would be for something like standard monsters having specific attack dices).
Given bugs in parsing pointed out in other comments – you should probably consider if another implementation approach would allow you to avoid them by its nature. One of the problem sources was use of regex in parsing (I am not great fan of it in general) – are you sure you need regex (or in that amount)? Maybe you can just "text".indexOf("D") and split the string by your local grammar in parts – writing simple parsers is really not that hard.
There is also another issue with regexes – if your class is anytime to parse untrusted input in current form it could be relatively easy to it make it use a lot of resources (see e.g. https://s0md3v.medium.com/exploiting-regular-expressions-2192dbbd6936) – although it could be simply circumvented by introducing some input length limit.
Another thing how you test randomness of dices (and how it didn’t catch the bug with new Random(0)) – notice that you have two methods for result in dice and only one of them is used in tests (the other seems unused but I guess that it might used somewhere else), usually this situation is a manifestation of a design flaw and it is a way for tests to tell us something valuable (pay attention to those) – here it is a way of saying that randomness seed is an external dependency and it is important enough to be identified and treated with respect.
Usually extracting seed and passing it as a dependency (in whatever way you manage them) is enough to write deterministic tests that check actual results in such case.
I’ll repeat it because it is so important: test for results that you care about, not what is easy to test at the moment (like testing that d4 roll can result in 1,2,3,4 values and not that result is between min and max – with the right seed you shouldn’t need hundreds of attempts) and don’t hesitate to change the design so that it is easy to test.
These are my main points, some related ideas that sparked in my head when seeing this code:
- die vs dice looks like a nice place for
compositepattern (I wouldn’t force it, simple is better than using arbitrary pattern – but if needed it this design could be leaned into) - if you wanted you could test distribution of results (a little over the top, I am aware, but if it was part of some safety critical software… then don’t use dices :D)
- you could use https://github.com/VerbalExpressions/JavaVerbalExpressions (I don’t think any use of regex is small enough, to not use it but its only my personal opinion) but better not use it at all
- Seeing Greg Burghardt comment I, personally don’t fully agree with constructors doing work being a bad thing (it surely can be bad) – I think that static parsing/validation (checking invariants) can be placed in constructor as a way to strengthen compile-time(ish) type safety and (again IMO) the root problem here is (mentioned at the beginning) mixture of ideas of parsing and rollable dice