Hello everyone, I am sure the dice game has been discussed numerous times on here. I am hoping to get some feedback on my take of the dice game. This was a homework assignment and I have completed the requirements. I am just wanting to see where I could have done better and where I could have saved time and effort. I appreciate any feedback that I receive.

The task asked to create a program to ask the user how many times they would like to roll the dice. Then using a random number generator roll the dice that many times and print out the values of each roll. After that create a histogram of the number of times a certain value was rolled(2-12).

        int input, rollTotal, count = 0;
        Scanner sc = new Scanner(System.in);
        System.out.print("Enter how many times you would like to roll "
                             + "the dice: ");
        input = sc.nextInt();

        //These are the arrays used to store the values generated by the loop 
        // instructions.
        int [] die1 = new int[input];
        int [] die2 = new int[input];
        int [] rollHist = new int[11];

        Random r = new Random();

        /*  This block of code generates the random numbers for die1 and die2 
         * and stores the values of each roll in the die1 and die2 arrays.
         * It then prints to the screen what each die represents for each roll 
         * of the dice.
         */
        for (int i = 0; i < die1.length; i++)
        {
            die1[i] = r.nextInt(6) + 1;
        }
        for (int j = 0; j < die2.length; j++)
        {
            die2[j] = r.nextInt(6) + 1;
        }


        for(int d = 0; d < input; d++)
        {
            System.out.print(count += 1);
            System.out.println(" : I rolled a " + die1[d] + " and a " + die2[d]);
            rollTotal = die1[d] + die2[d]; // This creates a total of the sum  
            if (rollTotal == 2) // for each roll of the dice. With the if 
            {
                rollHist[0] += 1; // statements it calculates how many times
            }
            if (rollTotal == 3) // a certain value is produced by a roll of the 
            {
                rollHist[1] += 1; // dice.
            }
            if (rollTotal == 4)
            {
                rollHist[2] += 1;
            }
            if (rollTotal == 5)
            {
                rollHist[3] += 1;
            }
            if (rollTotal == 6)
            {
                rollHist[4] += 1;
            }
            if (rollTotal == 7)
            {
                rollHist[5] += 1;
            }
            if (rollTotal == 8)
            {
                rollHist[6] += 1;
            }
            if (rollTotal == 9)
            {
                rollHist[7] += 1;
            }
            if (rollTotal == 10)
            {
                rollHist[8] += 1;
            }
            if (rollTotal == 11)
            {
                rollHist[9] += 1;
            }
            if (rollTotal == 12)
            {
                rollHist[10] += 1;
            }
        }
        /* This section calculates and displays a histogram for the value of 
         * each roll of the dice.  
         */
        System.out.println();
        System.out.println("Histogram of rolls:");

        /* In this final for loop the string variable histogram is created to 
         * change the total count for each value of the dice roll from 2-12 into
         * that many asterisk symbols. If there is no count for a certain value
         * then there are no asterisk printed to the screen.
         */
        count = 1;
        for (int k = 0; k < rollHist.length; k++)
        {
            System.out.print(count += 1);
            String histogram;
            histogram = new String(new char[rollHist[k]]).replace('\0', '*');
            System.out.println(" : " + histogram);
        }
        }
    }

First thing that leaps to my attention is this section:

if (rollTotal == 2) // for each roll of the dice. With the if 
{
    rollHist[0] += 1; // statements it calculates how many times
}
if (rollTotal == 3) // a certain value is produced by a roll of the 
{
    rollHist[1] += 1; // dice.
}
if ...

Notice that the second (and subsequent) if statements will always be evaluated, but we know that if rollTotal is two, only the first if block matters. A better way to express this is to use else clauses--something like this:

if (rollTotal == 2) // for each roll of the dice. With the if 
{
    rollHist[0] += 1; // statements it calculates how many times
}
else if (rollTotal == 3) // a certain value is produced by a roll of the 
{
    rollHist[1] += 1; // dice.
}
else if ...

Although if you're always testing the same variable, you may be able to use a switch statement as well. Something to consider.

But notice that every if block looks very similar to the others. The code may produce correct output, but patterns like this usually indicate that there's a more compact way to represent the operation, which IMO is better as long as it's not too hard to understand (for this exercise it won't be).

To get you started: Can you identify a relationship between the value of rollTotal and the integer you're using to index rollHist?

A few other comments:

int input

"Input" is where the value comes from, but in general, variable names should describe what the variable means more than anything else. In this case, something like totalRolls would make it much easier for someone else to read and understand your code (this includes future versions of yourself).

rollHist[0] += 1

Not really wrong, but rollHist[0]++ is more idiomatic. At least that's what I've observed.

/*  This block of code generates the random numbers for die1 and die2 
* and stores the values of each roll in the die1 and die2 arrays.

I don't know if you're supposed to be writing separate methods yet, but I'd like to observe that anywhere you feel that an introduction like this is appropriate, it is also probably appropriate to turn that section into its own method. Just as a general guideline.

Thank you for your responses gusano. I did think about the switch statement and I have done it before with a day/date program, but I opted for the if statements. You are definitely right, there is no need to waste time evaluating every if statement each time.

I think I understand your question and there is no relationship to the integer I am indexing in rollHist[]. I was thinking of making it match to 2-12 but then it would waste the first two indexes of the array. I hope this is what you meant and if not I would love to understand and learn.

I guess I have gotten into a bad habit of using the same variable name when I do the same task like get user input. I will have to keep that in mind in the future as it will be easier to keep track of items in a program for me as well as anyone else.

Can you go into further detail about turning the section into its own method, that is something I am not grasping at the moment. It is getting late.

Thank you again for you advice.

if (rollTotal == 10) {
   rollHist[8] += 1;
} (etc)

There's a pattern- the array index is always two less than the rollTotal, so all those ifs can eb replaced with a single line of code something like rollHist[rollTotal-2] += 1;

Comments
missed your post, but yup, that's the main point

also, for efficiëncy reasons (well, in a small piece of code like this, it won't matter that much, but still) you are better to use either a switch statement or an if else if structure.

to copy a bit of your code:

    if (rollTotal == 5)
                {
                    rollHist[3] += 1;
                }
                if (rollTotal == 6)
                {
                    rollHist[4] += 1;
                }
...

so .. what if rollTotal actually is equal to 5?
the value of rollHist[3] will be augmented, and you 'ld say, that's all she wrote. well, it isn't. she's still writing.
every check after that, for 6, 7, 8, all up to 12, is still made. sure, it won't return true and the values won't get updated, but still, your processor 'll take the time to make all those checks while actually, you already know it's redundant to do so.

two ways to prevent this:

switch statement:

switch(rollTotal){
  case 5: rollHist[3] += 1;
          break;
  case 6: rollHist[4] += 1;
          break;
  // other values
}

or else-if's

if ( rollTotal == 5){
  rollHist[3] += 1;
} else if (rollTotal == 6) { // will not be tested if it was 5
    rollHist[4] += 1;
}
..

. this would improve the efficiency of your code (well, it would matter more if you had lots and lots of possible results and more iterations, but still, nice to know, isn't it? :) )

but, and this might come in handy if you want to 'simplify' your code a lot: iterations and conditional statements are great, and they give you a lot of possibilities, but sometimes, well, you just don't need them.
for instance: you can rewrite the above without any if 's in that part at all. just replace that entire part with this:

rollHist[rollTotal-2] += 1;

that single line can replace that entire piece of code, making it a lot easier to read, and a lot lighter on execution/compile time.

EDIT: just noticed JamesCherrill already replied this :)

Edited 3 Years Ago by stultuske

Wow I still have so much to learn. Thank you both for that I have learned something new. I didn't realize that was possible. You reduced all of the if statements to a single line of code. That is a huge improvement.

A couple of minor comments:

for (int i = 0; i < die1.length; i++)  
{
   die1[i] = r.nextInt(6) + 1;
}
for (int j = 0; j < die2.length; j++)
{
die2[j] = r.nextInt(6) + 1;
}

no need for two loops, why not just

for (int i = 0; i < die1.length; i++) {
   die1[i] = r.nextInt(6) + 1;
   die2[i] = r.nextInt(6) + 1;
}

=====
rollHist has 11 elements, corresponding to the 11 possible rolls, but they are numbered 0-10 corresponding to rolls of 2-12. In my opinion the code would be clearer if rollHist was 13 elements with the count for a roll of 1 in [1] and the count for a roll of 12 is [12]. Yes, that means [0] and [1] are never used, but so what?

I see since die1 and die2 will always have the same length it does the same just to have one loop.

It did cross my mind to have the array index match the roll value, and I guess the only reason I didn't do that was because it wasted the first two places.Those two places will never be used in the program so it won't hurt the efficiency of the program and will make it clearer to read.

Thank you for your advice. Can I ask you James how long have you been programming?

Joined IBM as a trainee programmer in 1969, been doing it one way or another ever since, apart from a few wasted years in pure management roles. The irony is that by the mid 70's I understood maybe 50% of what there was to know about commercial programming; now it's more like 1% of what there is to know.

I am just getting started in programming and it does seem like a lot to learn. I guess things are changing at a faster pace and I can believe that with how technology exploded as I was growing up. The years you consider wasted in management was that because you have a greater desire to code than to manage?

I love building things, making things, designing things. And I love teaching. As CEO of a software house I wasted years in meetings with bankers, lawyers, shareholders etc, while the people working for me were doing all the things I wanted to do. On the other hand, the money was nice. In the end I went back to consulting in software design because being happy is more important than being rich.

Edited 3 Years Ago by JamesCherrill

I just would like to add something about using methods.
You can easily break up your code into methods by simple describing what it does. E.g. "Method takes user input and makes a roll and builds a histogram ...". The activities, separated by "and" word may be easily moved into separate methods (see the link in the end of my post).
One method should be responsible for only 1 operation and it's name should describe it's behavior. If your method name looks like it's too long ... take a closer look, if it can be divided into a few.

Take a look at this article: Single responsibility principle

perhaps this isnt a good place to post this query , but im doing so as my doubt relates to the if-else structure that has been talked about here...

in this code

    // compare by y-coordinate, breaking ties by x-coordinate
    public int compareTo(Point2D that) {
        if (this.y < that.y) return -1;
        if (this.y > that.y) return +1;
        if (this.x < that.x) return -1;
        if (this.x > that.x) return +1;
        return 0;
    }

Point2D is a class that deals with x,y co ordinates in a 2D plane , and it implements comparable() . my question is , that if one condition gets satisfied , lets suppose the 1st one, then will the next ones be checked?
i dont know if doing executing a return exits that method , hence this post. by the way, is return a method? i tried to google for it , but in every case, the return google thought im asking for wasnt what i was asking for.. i got stuff like returning arrays etc , but not what return means in java... so no luck there...

my apologies if im being vague , and making a post in the wrong place...

When you execute a return statement your method finishes at that point, and it does not execute any following statements (1)

(1) Unless you have a try/finally structure

Sorry for the delayed response,

@JamesCherrill I can totally agree on being happy with what you do is more important than the amount of money. That is why I have went back to school so that I can do something that makes me happy. The job I have now isn't very fulfilling anymore.

@Antenka Thank you for that article and my teacher has mentioned that also. I guess it hasn't sank in completely, but I am going to continue reading and learning to improve my coding skills.

Thank you again to everyone who responded and I hope to one day be able to help someone else out in the future.
I have one final question, should I mark this as solved now?

If your initial queston is answered then, yes, mark this solved. You can always start a new thread if ypu have a new question.

This question has already been answered. Start a new discussion instead.