5
\$\begingroup\$

This is my first time putting a code together for Java based on my own so i was hoping if you someone would be kind enough to review and provide feedback or constructive criticism on this code.

The goal is to roll 2 dice and 10k times. add pairs and display their frequency.

The code runs fine but maybe I am overlooking some logical error or a better way to do this.

/**
 * Use the Random Number generator to write a java application that simulates a pair of dice.
 * Throwing the pair of dice 10,000 times- Add the values for the pair
 * Print the frequency at the end of 10,000 runs
 * @author 
 *
 */
import java.util.Random;
public class diceSimulation {
 public static void main(String[] args) {
 /**
 * Declare variables and store dice max roll in Thousand
 */
 final int THOUSAND = 10000;
 int counter, dice1, dice2;
 //initiate an array with elements to keep count
 int [] diceValue = new int [7];
 // display welcome message. no other purpose but trial
 welcome ();
 // create new instance of random 
 Random rollDice = new Random();
 /**
 * Set counter to start from 1 and go till desired constant number
 * Rolling two separate dices and storing values in dice1 & dice 2 respectively
 */
 for (counter=1; counter<=THOUSAND;counter++){
 dice1=rollDice.nextInt(6) + 1;
 dice2=rollDice.nextInt(6) + 1;
 // If statement to check if values are the same or not
 if (dice1==dice2){
 // IF values are same then go into for loop and store value in array element
 for (int i=1; i<=6; i++){
 if (dice1 == i && dice2 == i){
 // add value for the number displayed into the array
 diceValue [i] += 1;
 }
 }
 }
 }
 // Display results totals of paired rolls 
 for (int a=1; a<diceValue.length; a++){
 System.out.println(" You rolled set of " + a + " " + diceValue[a] + " times");
 }
 }
 public static void welcome () {
 System.out.println("welcome to dice world!");
 }
}
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Jun 22, 2016 at 7:32
\$\endgroup\$
1
  • \$\begingroup\$ From the problem description, it seems like you want to add the two dice values, not search for pairs. The frequency distribution becomes more interesting if you do that. Why does your comment on line 2 say "add the values" otherwise? \$\endgroup\$ Commented Jun 22, 2016 at 9:17

1 Answer 1

7
\$\begingroup\$

Minor things

The java naming standard for classes is CamelCase (DiceSimulation instead of diceSimulation)

A constant of THOUSAND is not helpful, especially when the value is 10,000. A more meaningful name, say ROLLCOUNT, would be better.

Similarly, diceValue is not clear. It contains the counts for each double occurence, so perhaps, doubleCounts - note the plural for an array indicating that it contains multiple values.

We have many dice but each one is a die -> die1 and die2.

Comments that just restate the obvious code can be omitted
e.g.

/**
* Declare variables and store dice max roll in Thousand
*/
// create new instance of random 
// If statement to check if values are the same or not

Coding

We are working with 6 sided dice so we only need 6 entries in the array to store the double counts not 7 - it is more standard to adjust the index when outputting (e.g., add 1) than to make the arrays larger and use then as 1 based arrays..

We can easily store the number of sides in a variable (constant) to make consistency easier - a named value instead of a literal 6 in multiple places - and even allow us to use this for dice with other numbers of sides if we desired.

We can put the 'roll' functionality into a function so that to reduce duplication and allow us to change the implementation if desired (say for testing)

We check if dice1 == dice2 but then in the loop check if both dice1 == i && dice2 == i - if dice1 == dice2 then the second check can never fail.

If we index into the array, we don't need the loop

if (die1==die2){
 doubleCounts[die1-1]++;
}

If have sized the array to the number of elements then we should loop from 0 not 1 (and single character variable names, even for loops, are generally frowned upon)

System.out.format is a nicer way - simpler to use than the concatenation - for writing the output line

import java.util.*;
public class DiceSimulation {
 private static Random rollDice = new Random();
 public static void main(String[] args) {
 final int ROLLCOUNT = 10000;
 final int SIDES = 6;
 int counter, die1, die2;
 //an array with elements to keep count
 int [] doubleCounts = new int [SIDES];
 // display welcome message. no other purpose but trial
 welcome ();
 /**
 * Set counter to start from 1 and go till desired constant number
 * Rolling two separate dices and storing values in dice1 & dice 2 respectively
 */
 for (counter=1; counter<=ROLLCOUNT;counter++){
 die1=roll(SIDES);
 die2=roll(SIDES);
 if (die1==die2){
 doubleCounts[die1-1]++;
 }
 }
 // Display results totals of paired rolls 
 for (int idx=0; idx<doubleCounts.length; idx++){
 System.out.format(" You rolled set of %d %d times\n",(idx+1), doubleCounts[idx]);
 }
 }
 private static int roll(int sides){
 return rollDice.nextInt(sides) + 1;
 }
 public static void welcome () {
 System.out.println("welcome to dice world!");
 }
}
answered Jun 22, 2016 at 8:36
\$\endgroup\$
1
  • \$\begingroup\$ You got my vote at THOUSAND \$\endgroup\$ Commented Jun 23, 2016 at 8:28

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.