HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Imitate randomness of a dice roll

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
imitaterollrandomnessdice

Problem

I have written some code in Java that consists of two classes. I have tried my best and I want you to tell me if I can improve and how good this is.

Also, how important is OOP? Java is an object oriented language, so I have also tried my best to use it that way (although I don't know whether I have succeeded or not).

MainClass

import java.util.Scanner;
class MainClass{
public static void main(String args[]){
    RandomDice dice = new RandomDice();
    Scanner scan = new Scanner(System.in);

    System.out.print("Please enter the number of times you want to roll the dice:");
    int arg0 = scan.nextInt();

    dice.roll(arg0);
    }
}


RandomDice

/*This program will help you to imitate the randomness of a dice roll,
 *and to represent the frequency of each face appearing in a tabular form.
 */
import java.util.Random;

public class RandomDice{
private int diceRoll;
Random rand = new Random();

    /// the method below takes an argument arg0 which indicates the amount of times the         dice has to be rolled.
    public void roll(int arg0){
    diceRoll = arg0;
    int ObservationArray[] = {0,0,0,0,0,0};
    ObserveRandom(diceRoll,ObservationArray);
    DisplayResult(ObservationArray);
}

///this method creates random numbers from 0 to 5 and respectively stores them in the     array index stimulating a dice being rolled.
public void ObserveRandom(int arg1,int ObservationArray[]){
for(int counter=0;counter<arg1;counter++){
    ++ObservationArray[rand.nextInt(6)];
    }
}
///this method displays the data collected in a tabular form.
public void DisplayResult(int arg1[]){
    System.out.println("face\tfrequency");
    for(int counter=0;counter<arg1.length;counter++){
        System.out.println(1+counter+"\t"+arg1[counter]);
        }
    }
}

Solution

Thanks for submitting your code for review — it shows that you care about code quality and you're willing to improve and learn.

Note that OOP stands for object-oriented programming and does not have a plural form ;-)

Alexandre has given you some good advice and one possible implementation. I'd like to critique your code a bit more thoroughly and offer another, more lightweight possibility.

Criticism

Concept

An important idea in software development is that code should be reusable. Your code seems like a specific application of a general idea that applies not only to throwing the dice but just as well to tossing a coin or drawing a card (and gathering statistics).

So you should consider designing your code in a way that allows it to work for all these (and other) scenarios. That is to say, the number of elements in the input sequence (currently 6) and the element names themselves (currently {1,2,3,4,5,6}) should be configurable by the caller of the code.

Separation of concerns

I'll keep this part short since Alexandre has already elaborated on it. Just remember a few key points:

  • Never put any user interface related code into your core business logic (RandomDice class)



If you mix up these concerns, you'll find it hard to reuse your code for applications based on other UI frameworks than the Console (for instance, Swing, SWT or AWT or Servlets).

  • Instead of outputting data, use return values



Just replace occurrences of System.out.println with an appropriate return value (e.g. String)

  • Choose one responsibility per object and refactor others into new objects



The class that calculates the statistics should not be in charge of formatting the output — you need to introduce a new Collaborator (an object that encapsulates that responsibility)

Comments

  • Use them sparingly or they will cause clutter and be ignored



  • Check your spelling and grammar



  • amount of times should be number of times



  • stimulating should be simulating



-
Format them properly

For summaries above class and method definitions (don't put them between package and import but directly above the relevant code!) always use the same (Javadoc) syntax:

/** 
 * This program will help you to imitate the randomness of a dice roll,
 * and to represent the frequency of each face appearing in a tabular form.
 */


-
Choose better names to avoid writing a comment

Naming

-
Be expressive and avoid junk names such as arg0 or arg1

Instead of the following obsolete comment:

// / the method below takes an argument arg0 which indicates the amount of
// times the dice has to be rolled.
public void roll(int arg0) {
    diceRoll = arg0;


Why not get rid of the field diceRoll and write

public void roll(int numberOfTimes) {


-
Avoid stating the obvious (that applies to comments, too!)

Dice can reasonably be assumed to be random, so RandomDice is unnecessary

MainClass also states the obvious, just use Main

  • Follow conventions (methods and variables are camelCase, classes are PascalCase)



Also

-
Always specify access modifiers

Use the most restrictive access modifier possible (in your code, rand was accessible from other classes!):

-
Don't declare your arrays C-Style (keep the data type before the variable instead)

int ObservationArray[] = { 0, 0, 0, 0, 0, 0 }; // don't do this
int[] ObservationArray = { 0, 0, 0, 0, 0, 0 }; // this form is clearer


Alternative implementation

By refactoring step-for-step and slowly introducing changes, I arrived at the implementation shown below that aims to illustrate the points made in this answer. RandomDice was renamed to DistributionCalculator and is now generic (it can work with any type of elements, as illustrated in the Usage section). It accepts an Array of elements in the static convenience method with. After initialization, the frequencies can be calculated by calling calculateDistribution.

That method returns a Distribution which is the class that encapsulates the string formatting (I went ahead and made the output a bit more pretty).

The naming could probably be more accurate from a mathematical perspective — go ahead and rename things if you feel that these terms aren't quite correct (probably UniformDistributionCalculator or something would have been more exact).

The frequencies are now stored as longs to allow for bigger calculations, though that can be changed easily if you don't agree with the decision.

If something requires further explanation, go ahead and ask in the comments.

Usage

```
public class Main {
private static final Scanner scan = new Scanner(System.in);
private static long iterations;

public static void main(String args[]) {
System.out.print("Please enter the number of times you want to iterate: ");
iterations = scan.nextLong();
test(DistributionCalculator.with(1, 2, 3, 4, 5, 6));
test(DistributionCalculator.with

Code Snippets

/** 
 * This program will help you to imitate the randomness of a dice roll,
 * and to represent the frequency of each face appearing in a tabular form.
 */
// / the method below takes an argument arg0 which indicates the amount of
// times the dice has to be rolled.
public void roll(int arg0) {
    diceRoll = arg0;
public void roll(int numberOfTimes) {
int ObservationArray[] = { 0, 0, 0, 0, 0, 0 }; // don't do this
int[] ObservationArray = { 0, 0, 0, 0, 0, 0 }; // this form is clearer
public class Main {
    private static final Scanner scan = new Scanner(System.in);
    private static long iterations;

    public static void main(String args[]) {
        System.out.print("Please enter the number of times you want to iterate: ");            
        iterations = scan.nextLong();
        test(DistributionCalculator.with(1, 2, 3, 4, 5, 6));
        test(DistributionCalculator.with("hearts", "spades", "clubs", "diamonds"));
        test(DistributionCalculator.with(true, false, null));
    }

    private static <T> void test(DistributionCalculator<T> calculator) {
        System.out.println(calculator.calculateDistribution(iterations));
    }
}

Context

StackExchange Code Review Q#14551, answer score: 8

Revisions (0)

No revisions yet.