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!");
    }

}
\$\endgroup\$
  • \$\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\$ – JS1 Jun 22 '16 at 9:17
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!");
    }
}
\$\endgroup\$
  • \$\begingroup\$ You got my vote at THOUSAND \$\endgroup\$ – Heslacher Jun 23 '16 at 8:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.