current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 5 down vote favorite

I've written a basic dice program in Java which takes a number of faces then randomly selects a number within that boundary. Here is the code:

package com.georgegibson.diceroller;

import java.util.Random;
import java.util.Scanner;

public class DiceRoller
{
    public static void main(String []args)
    {
        Random dice = new Random();
        Scanner input = new Scanner(System.in);
        int faces;
        int result;

        System.out.println("Dice Roller\n");
        System.out.println("How many faces does the dice have?");
        faces = input.nextInt();
        result = dice.nextInt(faces) + 1;
        System.out.println("\nThe dice rolled a " + result + ".");
    }
}

Seen as I am pretty new to Java (I come from C/C++ land), I should think this can be improved, but how?

EDIT: I know I should have a different package name, but I am going to get that domain (georgegibson.com).

share|improve this question
up vote 5 down vote accepted

In your code you have declared a variable dice which is a Random. This is an indication that you have a mismatch between the object and the implementation. The fact that you call it 'dice' implies you want something that is a Dice.... not a Random. A Dice should be able to "encapsulate" itself.

I would suggest a clean Java object oriented design would have a Dice class, something like:

public class Dice {
    private final Random rand;
    private final int faces;

    public Dice(int faces) {
        this.rand = new Random();
        this.faces = faces;
    }

    public int roll() {
        return 1 + rand.nextInt(faces);
    }

}

Now you have a class that represents a dice, and you can roll that dice, and you now have a main method that looks like:

public static void main(String []args)
{
    Scanner input = new Scanner(System.in);


    System.out.println("Dice Roller\n");
    System.out.println("How many faces does the dice have?");
    Dice dice = new Dice(input.nextInt());
    System.out.println("\nThe dice rolled a " + dice.roll() + ".");
}

You can reuse that dice, or create other dice with a different number of faces.

Note, there's always going to be a discussion about the singular is "Die", and the plural is "Dice". One Die, many Dice.... but I say "who cares" call it a Dice.... Creating a class called "Die" is just wrong ;-)

share|improve this answer
up vote 4 down vote

Unlike in C, in Java people usually declare the variables when they actuall need them; for example:

System.out.println("How many...");
int faces = input.nextInt();
int result = dice.nextInt(faces) + 1;

Also, what comes to braces, more idiomatic style is 

public class DiceRoller {
    ...

    public static void main(String[] args) {
        ...
    }
}
share|improve this answer

Your Answer

 
discard

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