Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
public class diceGame {
    public static void main(String[] args) {
        int dice1;
        int dice2;
        int count = 0;
        int theSum = 0;
        int lowest = Integer.MAX_VALUE;
        int finalSum = 0;
        int diceSum;
        int totalSum=0;
        while (count < Integer.parseInt(args[0])) {
            count = count + 1;
            diceSum=0;
            theSum=0;
            while (diceSum!=7) {
                diceSum = 0;
                dice1 = 1 + (int) ((Math.random() * (6 - 1)) + 1);
                dice2 = 1 + (int) ((Math.random() * (6 - 1)) + 1);
                diceSum = dice1 + dice2;
                if (diceSum != 7) {
                    theSum = theSum + diceSum;
                }
                //System.out.println("the sum is "+theSum);
            }
            if (theSum > finalSum) {
                finalSum = theSum;
            }
            if (theSum < lowest) {
                lowest = theSum;
            }
            totalSum=totalSum+theSum;
        }
        double average=(double)totalSum/(Double.parseDouble(args[0]));
        System.out.println("After " + args[0] + " simulations: ");
        System.out.println("Biggest sum: " + finalSum);
        System.out.println("Smallest sum: " + lowest);
        System.out.println("The average is: "+average);

    }
}

I would like to execute this with much less code and want to learn other/better ways to do some of things I did, such as a random ints.

share|improve this question
    
as I commented on a previous question, (6 - 1)) + 1) == 6. To throw a dice, you want to use new Random().nextInt(6) + 1 –  njzk2 Feb 5 at 16:37
1  
I see you haven't upvoted any answers yet. Please do so if you've found any of them useful. You may also accept the answer you've found most helpful. –  Jamal Feb 6 at 16:11

5 Answers 5

Your main() function desperately needs to be busted up, for multiple reasons:

  • You use a lot of variables, all of them declared at the top of main(). A human mind is only good at keeping track of about 7 things at a time, so this code is hard to follow.
  • It violates the Single Responsibility Principle, by parsing the command line, throwing the dice, keeping the statistics, and printing the report. That's a lot of work for one function!
  • Object-oriented code is preferred in Java. A static function avoids object-oriented thinking.
  • It's unclear what you're simulating: what constitutes a trial, and what you mean by biggest/smallest sum.

A quick win in readability can be obtained by defining a Die class: die.toss() reads like English, whereas 1 + (int) ((Math.random() * (6 - 1)) + 1) doesn't.

class Die {
    // Since "throw" is a Java keyword, we use "toss"
    int toss() {
        return 1 + (int)(6 * Math.random());
    }
}

A good start to taming the variables would be to declare each of them in the tightest scope possible. For example, theSum, and diceSum are only relevant inside the loop. count is a loop counter, and it's much easier to recognize if you rewrite the loop as a for-loop. dice1 and dice2 are only relevant inside the inner loop, which is better expressed as a do-while loop.

for (int count = 0; count < Integer.parseInt(args[0]); count++) {
    int theSum = 0, diceSum;
    do {
        int dice1 = 1 + (int) ((Math.random() * (6 - 1)) + 1);
        int dice2 = 1 + (int) ((Math.random() * (6 - 1)) + 1);
        diceSum = dice1 + dice2;
        if (diceSum != 7) {
            theSum = theSum + diceSum;
        }
        //System.out.println("the sum is "+theSum);
    } while (diceSum != 7);
    …
}

Now a picture is beginning to emerge. The inner loop is what you would call a simulation trial. Based on that, I would define a DiceSimulation class with a runTrial() method. Everything else just falls into place around that core function. ☺

public class DiceSimulation {
    private int trials = 0,                 // Formerly count
                min = Integer.MAX_VALUE,    // Formerly lowest
                max = 0,                    // Formerly finalSum
                sum = 0;                    // Formerly totalSum

    private Die die1 = new Die(),
                die2 = new Die();

    /**
     * One trial consists of tossing a pair of dice until a sum of 7 is obtained.
     * The result of the trial is the sum of all tosses up to, but not including,
     * the toss that resulted in 7.
     */
    public int runTrial() {
        int trialSum = 0, pairSum;           // Formerly theSum and diceSum
        while (7 != (pairSum = die1.toss() + die2.toss())) {
            trialSum += pairSum;
        }

        if (trialSum > max) {
            max = trialSum;
        }
        if (trialSum < min) {
            min = trialSum;
        }
        sum += trialSum;
        trials++;
        return trialSum;
    }

    public void report() {
        System.out.println("After " + trials + " simulations: ");
        System.out.println("Biggest sum: " + max);
        System.out.println("Smallest sum: " + min);
        System.out.println("The average is: " + (double)sum / trials);
    }

    public static void main(String[] args) {
        int trials = Integer.parseInt(args[0]);

        DiceSimulation sim = new DiceSimulation();
        for (int count = 0; count < trials; count++) {
            sim.runTrial();
        }
        sim.report();
    }
}
share|improve this answer
1  
I prefed prefixing fields with this.. This makes it a lot more visible what is going on. For example the sum+= trailSum;.. this.sum+= trailSum makes obvious a field is getting updated –  RobAu Feb 6 at 8:14
    
I liked the idea of using Die.toss but you can use a single die object here. Another thing, 7 != (pairSum = die1.toss() + die2.toss() makes code slightly more cryptic by having multiple responsibility together. Rather have a do while with do { pairSum = die1.toss() + die2.toss(); ... }while(pairSum!=7) –  Archit Jain Feb 6 at 18:01
    
I prematurely posted comment. Edited . see the the comment now. –  Archit Jain Feb 6 at 18:04
    
@ArchitJain Note that trialSum should sum tosses before the 7, excluding the 7. –  200_success Feb 6 at 18:06
1  
umm.. corrrect..i missed that. On using a single die, pairSum = die.toss() + die.toss() Doesn't this works? –  Archit Jain Feb 6 at 18:11
  • As per Java naming convention, user-defined types should be capitalized. In this case, diceGame should be DiceGame.

  • Prefer to have variables defined in the lowest scope possible. This is especially useful in not having to worry about whether or not a variable is still in use.

    Since dice1 and dice2 are only used within the loop, you can just initialize them in there:

    // these can be removed
    int dice1;
    int dice2;
    
    // ...
    
    int dice1 = 1 + (int) ((Math.random() * (6 - 1)) + 1);
    int dice2 = 1 + (int) ((Math.random() * (6 - 1)) + 1);
    
  • These:

    theSum = theSum + diceSum;
    totalSum = totalSum + theSum;
    

    can be rewritten as this:

    theSum += diceSum;
    totalSum += theSum;
    

    This works in similar cases when you're accumulating a total with an arithmetic operator.

  • theSum is not a descriptive name. The sum of what? Based on the code alone, it's hard to tell exactly what it's used for, especially when you have diceSum, finalSum, and totalSum.

  • I agree with @200_success' suggestion about creating a Die class. With that, you can create any number of Die objects instead of int variables. You can even have a data structure, such as an array, that holds these objects. You could have one array of Die objects instead of having to define multiple ones, especially if you end up needing as many as, say, 100 dice.

share|improve this answer

I would like to add one more thing, in while loop, the type conversion is happening on each iteration.

while (count < Integer.parseInt(args[0])) {
   count = count + 1;

It should be done only once:

int rolls = Integer.parseInt(args[0]);

while (count < rolls) {
   count = count + 1;

Which would be better written as a for loop:

int rolls = Integer.parseInt(args[0]);
for (int count = 0; count < rolls; count++) {
share|improve this answer
1  
As it depends on Math.random() I don't see how it could be done only once. The only way to avoid it is to use @rolfl's suggestion of using the Random class, which I highly recommend. –  Simon André Forsberg Feb 5 at 15:26
    
@SimonAndréForsberg You misunderstood this answer, see my edit –  amon Feb 5 at 15:28
1  
Actually, 200_success already fixed this in his answer without specially mentioning it. –  amon Feb 5 at 15:31
    
@amon Good edit, now I see. +1. –  Simon André Forsberg Feb 5 at 15:31

200_success has suggested that you change the readability of the line:

dice1 = 1 + (int) ((Math.random() * (6 - 1)) + 1);

to be:

1 + (int)(6 * Math.random());

(but then continues to use your version in his subsequent code)

This is a good suggestion, but, more than that, you have to do that (or something similar) because your version of the dice-throw does not work.

Math.random() * (6 - 1)

will produce a value from 0.0 to 4.9999....

Putting it back in the full context, you do:

1 + (int) ((Math.random() * (6 - 1)) + 1);

Which will produce the values 2 through 6 (but never 1).

Using random numbers is surprisingly easy to get wrong... Part of the problem is that Math.random() works in the double domain from 0.0 to 0.9999....

I strongly recommend using the java.util.Random class which has some easier methods to use which at least are closer to the domain of values you want, int.

Random randomsource = new Random();
....
    dice1 = 1 + randomsource.nextInt(6);
    dice2 = 1 + randomsource.nextInt(6);

The above will create 1 of 6 different random integers (0 though 5), and will add 1 to the result. Much simpler.

share|improve this answer

In addition to what others already have mentioned, I just have a few things to add:

  • Magic numbers. Why 7? Where does that come from? Why 6? (There are 20-sided dice as well). Declare these numbers as constants, usually any number that's not 0 or 1 (and even those too sometimes, for when you don't have a good reason for using 0 or 1).

    private static final int DIE_SIDES = 6;
    private static final int TARGET_NUMBER = 7;
    

    Then instead of writing 6 or 7 in other places of your code, write DIE_SIDES or TARGET_NUMBER instead. Also, if you want to make it more flexible later (and more object oriented as @200_success suggests - which I agree on), you could make these instance variables instead of static variables by simply removing the static keyword.

  • Consistent spacing. I find that it is harder to read:

    totalSum=totalSum+theSum;
    

    than to read

    totalSum = totalSum + theSum;
    

    You use good spacing in most of your code. Stick to that.

  • Your use of division and the argument. On one place in the code, you parse args[0] as integer, but here you parse it as double: (also note the fixed spacing!)

    double average = (double)totalSum / (Double.parseDouble(args[0]));
    

    It seems to me quite clear that you always want to parse it as an integer, as it is the number of times to throw. And it doesn't make sense to throw 3.47 times, or 9.17 times, does it? I understand you wrote this line to avoid the resulting number becoming an integer, as int / int == int in Java.

    But as double / int == double and int / double == double you only need to typecast one of them to double, for simplicity, let's go with totalSum. And as @user36245 pointed out, you should only parse the number once and use it as a variable, this will also improve the readability of the line.

    int numberOfThrows = Integer.parseInt(args[0]);
    ...
    double average = (double)totalSum / numberOfThrows;
    

    Now it is easier to tell what we are dividing :)

  • Shorter addition. Instead of writing totalSum = totalSum + theSum you can write totalSum += theSum. Also, for cases when you just want to increase by one, as in count = count + 1; you can use count++;

I must say though, as I want to give a compliment also, that you're code is quite well formatted. You know where to put braces and parenthesis and you know how to indent your code properly, good! Keep on learning more things!

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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