patternjavaMinor
Imitate randomness of a dice roll
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
RandomDice
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
Separation of concerns
I'll keep this part short since Alexandre has already elaborated on it. Just remember a few key points:
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).
Just replace occurrences of
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
-
Format them properly
For summaries above class and method definitions (don't put them between
-
Choose better names to avoid writing a comment
Naming
-
Be expressive and avoid junk names such as
Instead of the following obsolete comment:
Why not get rid of the field
-
Avoid stating the obvious (that applies to comments, too!)
Also
-
Always specify access modifiers
Use the most restrictive access modifier possible (in your code,
-
Don't declare your arrays C-Style (keep the data type before the variable instead)
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.
That method returns a
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
The frequencies are now stored as
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
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 (
RandomDiceclass)
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 timesshould benumber of times
stimulatingshould besimulating
-
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 arg1Instead 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 writepublic void roll(int numberOfTimes) {-
Avoid stating the obvious (that applies to comments, too!)
Dice can reasonably be assumed to be random, so RandomDice is unnecessaryMainClass also states the obvious, just use Main- Follow conventions (methods and variables are
camelCase, classes arePascalCase)
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 clearerAlternative 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 clearerpublic 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.