trjhtr

In order to keep my coding skills fresh, I am going to be implementing a simple D20 RPG engine/library to use in creating a text-based game. I am coding in with IntelliJ using Java 8. I'm focusing on best practices, especially testing and documentation as I'd like to start contributing to Open Source projects. I finished my first class, DiceRoller and am looking for input and advice.

Main Class:

import java.util.Random;

/**
 * Class for rolling dice
 */
public class DiceRoller 

 enum Sides 
 TWO (2),
 FOUR (4),
 SIX (6),
 EIGHT (8),
 TEN (10),
 TWELVE (12),
 TWENTY (20),
 HUNDRED (100);

 private final int sides;

 Sides(int sides)
 this.sides = sides;
 

 public int numSides() 
 return this.sides;
 
 

 private Random rnd;

 public DiceRoller() 
 this.rnd = new Random((long) Math.random());
 

 /**
 *
 * Roll a two-sided die
 *
 * @return the result of rolling a two-sided die
 */
 public int d2() 

 return this.between(Sides.TWO.numSides());

 

 /**
 *
 * Roll n two-sided dice
 *
 * @param n
 * @return the result of rolling n two-sided dice
 */
 public int d2(int n) 

 return this.between(n, n * Sides.TWO.numSides());

 

 /**
 *
 * Roll a four-sided die
 *
 * @return the result of rolling a four-sided die
 */
 public int d4() 

 return this.between(Sides.FOUR.numSides());

 

 /**
 *
 * Roll n four-sided dice
 *
 * @param n
 * @return the result of rolling n four-sided dice
 */
 public int d4(int n) 

 return this.between(n, n * Sides.FOUR.numSides());

 

 /**
 *
 * Roll a six-sided die
 *
 * @return the result of rolling a six-sided die
 */
 public int d6() 

 return this.between(Sides.SIX.numSides());

 

 /**
 *
 * Roll n six-sided dice
 *
 * @param n
 * @return the result of rolling n six-sided dice
 */
 public int d6(int n) 

 return this.between(n, n * Sides.SIX.numSides());

 

 /**
 *
 * Roll an eight-sided die
 *
 * @return the result of rolling a eight-sided die
 */
 public int d8() 

 return this.between(Sides.EIGHT.numSides());

 

 /**
 *
 * Roll n eight-sided dice
 *
 * @param n
 * @return the result of rolling n eight-sided dice
 */
 public int d8(int n) 

 return this.between(n, n * Sides.EIGHT.numSides());

 

 /**
 *
 * Roll a ten-sided die
 *
 * @return the result of rolling a ten-sided die
 */
 public int d10() 

 return this.between(Sides.TEN.numSides());

 

 /**
 *
 * Roll n ten-sided dice
 *
 * @param n
 * @return the result of rolling n ten-sided dice
 */
 public int d10(int n) 

 return this.between(n, n * Sides.TEN.numSides());

 

 /**
 *
 * Roll a twelve-sided die
 *
 * @return the result of rolling a twelve-sided die
 */
 public int d12() 

 return this.between(Sides.TWELVE.numSides());

 

 /**
 *
 * Roll n twelve-sided dice
 *
 * @param n
 * @return the result of rolling n twelve-sided dice
 */
 public int d12(int n) 

 return this.between(n, n * Sides.TWELVE.numSides());

 

 /**
 *
 * Roll a twenty-sided die
 *
 * @return the result of rolling a twenty-sided die
 */
 public int d20() 

 return this.between(Sides.TWENTY.numSides());

 

 /**
 *
 * Roll n twenty-sided dice
 *
 * @param n
 * @return the result of rolling n twenty-sided dice
 */
 public int d20(int n) 

 return this.between(n, n * Sides.TWENTY.numSides());

 

 /**
 *
 * Roll a hundred-sided die
 *
 * @return the result of rolling a hundred-sided die
 */
 public int d100() 

 return this.between(Sides.HUNDRED.numSides());

 

 /**
 *
 * Roll n hundred-sided dice
 *
 * @param n
 * @return the result of rolling n hundred-sided dice
 */
 public int d100(int n) 

 return this.between(n, n * Sides.HUNDRED.numSides());

 

 /**
 * Roll an s-sided die
 *
 * @param s
 * @return the result of rolling an s-sided die
 */
 public int customDie(int s) 

 return this.between(s);

 

 /**
 * roll n s-sided dice
 *
 * @param s
 * @param n
 * @return the result of rolling n s-sided dice
 */
 public int customDie(int s, int n) 
 return this.between(n, n * s);
 

 /**
 * Helper function to generate numbers between 1 and max inclusive
 *
 * @param max
 * @return an integer between 1 and max
 */
 private int between(int max) 
 return this.between(1, max);
 

 /**
 *
 * Helper function to generate numbers between min and max inclusive
 *
 * @param min
 * @param max
 * @return an integer between min and max
 */
 private int between(int min, int max) 
 return this.rnd.nextInt(max - min + 1) + min;
 

Test Class:

import java.util.List;

import static org.junit.Assert.*;

public class DiceRollerTest 

 static final int NUM_ROLLS = 100;
 static final int NUM_DICE = 10;
 static final int NUM_SIDES = 37;

 DiceRoller roller;

 List<Integer> rolls;

 @Before
 public void init() 

 roller = new DiceRoller();
 rolls = new ArrayList();

 

 @org.junit.Test
 public void d2() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d2());
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= 1 && roll <= 2);
 

 

 @org.junit.Test
 public void d21() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d2(NUM_DICE));
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= NUM_DICE && roll <= (2 * NUM_DICE));
 

 

 @org.junit.Test
 public void d4() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d4());
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= 1 && roll <= 4);
 


 

 @org.junit.Test
 public void d41() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d4(NUM_DICE));
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= NUM_DICE && roll <= (4 * NUM_DICE));
 

 

 @org.junit.Test
 public void d6() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d6());
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= 1 && roll <= 6);
 

 

 @org.junit.Test
 public void d61() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d6(NUM_DICE));
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= NUM_DICE && roll <= (6 * NUM_DICE));
 

 

 @org.junit.Test
 public void d8() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d8());
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= 1 && roll <= 8);
 

 

 @org.junit.Test
 public void d81() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d8(NUM_DICE));
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= NUM_DICE && roll <= (8 * NUM_DICE));
 

 

 @org.junit.Test
 public void d10() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d10());
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= 1 && roll <= 10);
 

 

 @org.junit.Test
 public void d101() 
 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d10(NUM_DICE));
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= NUM_DICE && roll <= (10 * NUM_DICE));
 

 

 @org.junit.Test
 public void d12() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d12());
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= 1 && roll <= 12);
 

 

 @org.junit.Test
 public void d121() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d12(NUM_DICE));
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= NUM_DICE && roll <= (12 * NUM_DICE));
 

 

 @org.junit.Test
 public void d20() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d20());
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= 1 && roll <= 20);
 

 

 @org.junit.Test
 public void d201() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d20(NUM_DICE));
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= NUM_DICE && roll <= (20 * NUM_DICE));
 

 

 @org.junit.Test
 public void d100() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d100());
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= 1 && roll <= 100);
 

 

 @org.junit.Test
 public void d1001() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.d100(NUM_DICE));
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= NUM_DICE && roll <= (100 * NUM_DICE));
 

 

 @org.junit.Test
 public void customDie() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.customDie(NUM_SIDES));
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= 1 && roll <= NUM_SIDES);
 

 

 @org.junit.Test
 public void customDie1() 

 for(int i = 0; i < NUM_ROLLS; i++)
 rolls.add(roller.customDie(NUM_SIDES, NUM_DICE));
 

 for(Integer r : rolls) 
 int roll = r.intValue();
 assertTrue(roll >= NUM_DICE && roll <= (NUM_SIDES * NUM_DICE));
 

 

My opinion:

1 - The sides enum doesn’t contribute much to the overall solution. It adds more complexity than it should so far. You could make it Die with the rolling functionality of the given die within it.

2 - The statistics of rolling multiple dice are wrong. When you add up more than one die you will get an even distribution of results. Like 2D6 will give you the same likelihood for the result for 12 as for 7. That isn’t the case with regular physical dice. 12 is just given with 6+6 which is the likelihood of 1/36. 7 is 1+6, 2+5, 3+4, 4+3, 5+2, 6+1 which is the likelihood of 6/36. So just role the die multiple times and sum it up. At least for smaller amounts of dice. For very large amounts I think there are “tricks” in statistics to help you.

3 - I believe that (long) Math.random() will always return 0 as Math.random() gives you a number between 0 and 1.

I know what a Dice is and I know people that can use the dice to roll it, but calling them "DiceRoller" would be strange and I cannot imagine what a DiceRoller would be otherwise, so I would suggest renaming the class in a way that isn't that much concerned about the behaviour of the class.

I guess you're working here on your own, but I would suggest getting used to the practice of making everything final if possible. In this case, your class is not final and I could extend it, but did you design your class to be extended? Probably not and thus you should make it final. In a project with more than one guy, you should prevent the others as much as reasonable from doing bad stuff with your code. Otherwise, they will complain about it, even if it were their mistake.

If you have no comment for a parameter or other stuff, remove the @param. Redundancy isn't nice.

You've hardcoded the dices and this is bad for you and the user of your class. If I were to use your class, I would wonder, why there isn't a, for example, thousand dice. It doesn't differ from the others based on the logic. It would be just another number, but you hardcoded the different variants into the enum and into the methods names. Additionally, you have code duplication with that practice. Your d4 method, for example, is nearly the same as the d6 method. You should make it more general.