Imitate randomness of a dice roll

Question

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]);
        }
    }
}

Answer 1

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<K,V> 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("hearts", "spades", "clubs", "diamonds"));
        test(DistributionCalculator.with(true, false, null));
    }

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

---

Output

 Please enter the number of times you want to iterate:  200000

 element             frequency
 1                   33129
 2                   33156
 3                   33539
 4                   33469
 5                   33346
 6                   33361

 element             frequency
 hearts              50100
 diamonds            49857
 spades              49752
 clubs               50291

 element             frequency
 null                66560
 false               66726
 true                66714

---

Calculation of frequencies

public class DistributionCalculator<T> {

    private static final Random random = new Random();
    private final Map<T, Long> map = new HashMap<T, Long>();
    private final T[] keys;

    public static <T> DistributionCalculator<T> with(T... elements) {
        return new DistributionCalculator<T>(elements);
    }

    private DistributionCalculator(T... elements) {
        keys = elements;
        for (T key : keys) {
            map.put(key, Long.valueOf(0));
        }
    }

    public Distribution<T, Long> calculateDistribution(long numberOfIterations) {
        for (long i = 0; i < numberOfIterations; i++) {
            T key = keys[random.nextInt(keys.length)];
            map.put(key, map.get(key) + 1);
        }
        return new Distribution<T, Long>(map);
    }
}

---

Result formatting

public class Distribution<K, V> {
    private final Map<K, V> map;
    private final String table;

    public Distribution(Map<K, V> distribution) {
        map = distribution;
        table = generateTable();
    }

    public Map<K, V> asMap() {
        return map;
    }

    @Override
    public String toString() {
        return table;
    }

    private String generateTable() {
        final String line = "%n%-20s%s";
        String result = String.format(line, "element", "frequency");
        for (K key : map.keySet()) {
            // it would be better to use a StringBuilder if you have a lot of keys
            result += String.format(line, key, map.get(key));
        }
        return result;
    }
}

Answer 2

Your code is clear and readable. You are asking about how much your code fit with OOP and this is a good mindset.

I won't be writing here abstract and theoretical concepts about OOP or design patterns, but just the first ideas which came up to my mind while I read your code.

1) First, about naming

In Java, method, variable and parameter names all start with a lower case letter (as opposed of methods in C++ for instance). You need to change the variable name ObservationArray and the method names ObserveRandom and DisplayResult accordingly.

Also, you should provide a better name for the integer variable which stores the number of dice rolls, like, for instance, diceRollCount instead of arg0. The same remark applies for code wihtin the RandomDice object.

2) About design

Separation of concern and specialization of objects is one of the core concept in OOP. Here, your code is showing a rather procedural approach:

1. Ask the user how many dice rolls to do
2. Create an ObservationArray
3. Roll dice as many time as expected
4. Print the result

In OOP, you need rather to think in term of components which you can see as black boxes with a limited responsibility and which share well-established relationships with other objects. Other objects should not rely on the concrete implementation of these black boxes but only on the contract which describes how to use them and for which intent.

With this in mind, we can extract a couple of conceptual objects for your purpose:

1. An object which state will represent the result of the computation (=the dice rolls).
2. An object which will produce the computation result (the "computer" object)
3. An object which will provide the computer with the number of times it should roll dice.
4. An object which will consume the computation result.

With this separation of concern, you can see that it is not that important where the number of times to roll dice is taken from : be it from the keyboard input, from a file, from a program argument, a text field in a GUI or whatever. Where you print the result can also be abstracted. What emerges also from this design is the state object which purpose is to hold the computation result. It is an example of what we could call a domain object, which here contains the value-added by your business logic and is designed on purpose. It is used to convey information between the different components of your architecture : the producer (which computes the result) and the consumer (which prints the result or do whatever else).

To make it more concrete, we could end up with the following objects:

class RollCountProvider {

  public int getRollCount() { ... }
}

class DiceRollResult {

  private final Map<Integer, Integer> frequencies;

  public DiceRollResult(Map<Integer, Integer> frequencies) {
    this.frequencies = frequencies;
  }

  public int getFrequency(int face) { 
    // Check bounds
    return frequencies.get(face);
  }
}

class DiceRoller {

  private static final int DICE_FACES_COUNT = 6;

  public static DiceRollResult roll(RollCountProvider rcProvider) { 
    final Map<Integer, Integer> frequencies = new HashMap<Integer, Integer>();

    for (int i = 0 ; i < rcProvider.getRollCount() ; i++) {
      int face = rand.nextInt(DICE_FACES_COUNT); // Avoid magic number
      Integer frequency = frequencies.get(face);

      if (null == frequency) // Take care to check for null reference
        frequency = 0;       // use autoboxing

      frequencies.put(face, ++frequency);
    }

    return new DiceRollResult(frequencies);
  }
}

class RollResultPrinter {

  public static void printResult(DiceRollResult result) { ... }
}

Few remarks about these classes:

1. As you can see, some methods used the static modifier since their class does not hold any particular state.
2. You do not need to stick to an array of integer but can use a Map instead.
3. Your domain object DiceRollResult should be immutable (which is not in this implementation: this is a design flaw. The object should copy the map provided to the constructor to ensure immutability).
4. Avoid magic number (always prefer constants to literals when the inference of their meaning from the context is not straightforward).

All of this is rather overdesigned for this particular program, but I hope it will help you to build your own insights about OOP design.

Answer 3

1. System.out.println(1 + counter + "\t" + arg1[counter]);
   

   Using + as addition in a println is fragile. A small modification could broke the addition:

   System.out.println("\t" + 1 + counter + "\t" + arg1[counter]);
   

2. Proper and consistent indentation would improve the readability a lot.

3. Consider using Guava's MultiSet for the counting.