Generating random numbers

Question

I'm trying to refactor the following code and could use some suggestions. Basically, three functions generating random numbers in different ways are called:

import java.util.Random;
public final class JP_RandomNumbers {
 /**
 * @param aArgs
 */
 public static void main(String[] aArgs) {
 genRandomNumbers1();
 genRandomNumbers2();
 genRandomNumbers3();
 }
 private static void genRandomNumbers1() {
 log("Generating 10 random integers in range 0..99.");
 //note that a single Random object is reused here
 Random randomGenerator = new Random();
 for (int idx = 0; idx < RANGE ; ++idx) { 
 int randomInt = randomGenerator.nextInt(100);
 log("Generated :" + randomInt);
 }
 log("Done.");
 }
 private static void genRandomNumbers2() { 
 //Generating random numbers in a specific range
 int START = 1;
 int END = 10;
 Random random = new Random();
 for (int idx = 1 ; idx <= 10 ; ++idx) {
 showRandomInteger(START, END, random);
 }
 log ("Done");
 }
 private static void showRandomInteger(int aStart, int aEnd, Random aRandom) { 
 if(aStart > aEnd) throw new IllegalArgumentException("Start cannot exceed End.");
 //get the range, casting to avoid overflow problems
 long range = (long)aEnd - (long) aStart + 1;
 //compute a fraction of the range , 0 <= frac <= range
 long fraction = (long) (range * aRandom.nextDouble());
 int randomNumber = (int)(fraction + aStart);
 log("Generated : " + randomNumber);
 }
 private void genRandomNumbers3() { 
 double MEAN = 100.0f;
 double VARIANCE = 5.0f;
 for(int idx = 1; idx <= 10; ++idx){
 log("Generated : " + String.valueOf(getGaussean(MEAN,VARIANCE))); 
 }
 log ("Done");
 }
 private double getGaussean(double aMean, double aVariance){
 return aMean + fRandom.nextGaussian() * aVariance;
 }
 //Private 
 private static final int RANGE = 10;
 private Random fRandom = new Random();
 private static void log( String aString) { 
 System.out.println(aString);
 }
}

Output:

Generating 10 random integers in range 0..99.
Generated :74
Generated :99
Generated :75
Generated :15
Generated :32
Generated :31
Generated :96
Generated :61
Generated :80
Generated :89
Done.
Generated : 2
Generated : 4
Generated : 1
Generated : 9
Generated : 9
Generated : 5
Generated : 9
Generated : 4
Generated : 3
Generated : 7
Done
Generated : 96.16324820042756
Generated : 97.4298343909747
Generated : 105.18654272922421
Generated : 90.56523225378042
Generated : 100.26474837892167
Generated : 99.13288502577953
Generated : 105.99193834986883
Generated : 101.21081536019025
Generated : 99.48291677422242
Generated : 100.55524156214658
Done

Answer 1

The number generation technique is generally sound. I just have the following minor suggestions:

• The idiomatic way to repeat something 10 times is

  for (int idx = 0 ; idx < 10 ; ++idx) { ... }
  

• Use the fRandom instance variable; you shouldn't need to create new Random() in each function.

• You hard-coded values in each function; those constants should probably be pulled out to the class level.
• showRandomInteger() should be changed to return a random value instead of logging it.

• The compiler will implicitly do String.valueOf() for you, so you can just write

  log("Generated : " + getGaussean(MEAN,VARIANCE));
  

• getGaussean() should be spelled getGaussian().

• Nikhil: thanks, didnt notice I was creating multiple random objects

Answer 2

public final class JP_RandomNumbers {

Classes are normally UpperCamelCase without underscores.

---

/**
 * @param aArgs
 */
public static void main(String[] aArgs) {

Why is the JavaDoc part empty? Either remove it or fill in some info. Leaving an empty JavaDoc comment laying around feels like the coder is lazy.

Also, why is it called aArgs? Normally it's only args.

---

genRandomNumbers1();
genRandomNumbers2();
genRandomNumbers3();

These are not good function names. Whenever you have the urge to suffix functions, classes or variable names with numbers (because they're colliding) you're doing something wrong.

---

for (int idx = 0; idx < RANGE ; ++idx) {

I like your use of idx instead of i, though, count would be a better name in this case.

---

int randomInt = randomGenerator.nextInt(100);
log("Generated :" + randomInt);

This could be shortened, there's no need to store the value first if it is not going to be reused.

---

//Generating random numbers in a specific range
int START = 1;
int END = 10;

Local and/or instance variables are lowerCamelCase. UPPER_SNAKE_CASE is preserved for (static) constants and enum members. These are neither static nor final.

---

private static void showRandomInteger(int aStart, int aEnd, Random aRandom) {

First: This function would be better named logRandomInteger.

Second: Why are you prefixing every argument with a? That's not necessary in any way and will only complicate things in the long run. Drop it.

---

if(aStart > aEnd) throw new IllegalArgumentException("Start cannot exceed End.");

Very good. Though, is a negative start allowed? Are both be allowed to be negative? If yes, this should be documented.

---

double MEAN = 100.0f;
double VARIANCE = 5.0f;

Same here with the names. And you're storing floats in doubles, drop that f after the number.

---

for(int idx = 1; idx <= 10; ++idx){

You're inconsistent with your use of constants...in the first function the limit was RANGE.

---

private Random fRandom = new Random();

As I said, you should drop such prefixes. If it is unclear from the context what variable is used, you're doing something wrong (and there's always this).

---

From time to time you have an empty line before the closing bracket of a function...normally there is none. At least be consistent.

---

At the moment, your class looks like this:

import java.util.Random;
public final class JP_RandomNumbers {
 public static void main(String[] aArgs) { }
 private static void genRandomNumbers1() { }
 private static void genRandomNumbers2() { }
 private static void showRandomInteger(int aStart, int aEnd, Random aRandom) { }
 private void genRandomNumbers3() { }
 private double getGaussean(double aMean, double aVariance){ }
 //Private 
 private static final int RANGE = 10;
 private Random fRandom = new Random();
 private static void log( String aString) { }
}

This is quite messy. Group functions and variables by their visibility and if they're instances or not.

Also consider extracting everything away from your Main class into a separate class that's easier to use. Putting functionality into the Main class can be the start of an unholy mess. Advice I always try to keep in ming:

Your main function should tell you what the program does, not how it does it.

• Nikhil: thx, that was pretty thorough. the a in aArgs, aStart etc is to indicate that this is an argument to the function.