Please review this code

[guptan]

My problem is sometimes the getRandom(int low, int high) function
returns negative values.
Please review this code


import java.util.Random;

public class RNG
{
private Random random;

public RNG() {
if(random == null){
random = new Random(System.currentTimeMillis());
}
random.setSeed(System.currentTimeMillis());
}

public void setSeed(long l) {
random.setSeed(l);
}

public int getRandom(int max) {
return (max == 0) ? 0 : (Math.abs(random.nextInt() % max));
}

///get random in range int the closed range [low, mid]
public int getRandom(int low, int high) {
if(low > high) {
int t = low;
low = high;
high = t;
}else if(low == high) {
return low;
}
int range = (high - low) + 1;
return low + ((range * getRandom(100))/100);
}
}

 

[Joshua Cranmer]

public int getRandom(int max) {
return (max == 0) ? 0 : (Math.abs(random.nextInt() % max));
}

That code is very, very wrong for getting a uniform random number in the
range [0, max]. Which is why it's generally not a good idea to reinvent
something already part of the core Java API. If you need to know why,
find 2^31 % 3.

And, FWIW, you'll also run into the problem with lower-bit short cycles
that arise out of linear congruential pseudorandom number generators.

 

[Andre Silaghi]

My problem is sometimes the getRandom(int low, int high) function
returns negative values.
Please review this code


import java.util.Random;

public class RNG
{
private Random random;

public RNG() {
if(random == null){
random = new Random(System.currentTimeMillis());
}
random.setSeed(System.currentTimeMillis());
}

public void setSeed(long l) {
random.setSeed(l);
}

public int getRandom(int max) {
return (max == 0) ? 0 : (Math.abs(random.nextInt() % max));
}

///get random in range int the closed range [low, mid]
public int getRandom(int low, int high) {
if(low > high) {
int t = low;
low = high;
high = t;
}else if(low == high) {
return low;
}
int range = (high - low) + 1;
return low + ((range * getRandom(100))/100);
}
}

hm what about ^^ elswhere, if low is < 0 it is possible to get a
negative value...

if ( (low + ((range * getRandom(100)) / 100) ) < 0)
return (low + ((range * getRandom(100))/100) ) * (-1);
else
return low + ((range * getRandom(100))/100);

 

[Mike Amling]

My problem is sometimes the getRandom(int low, int high) function
returns negative values. ....
public int getRandom(int low, int high) {
if(low > high) {
int t = low;
low = high;
high = t;
}else if(low == high) {
return low;
}
int range = (high - low) + 1;
return low + ((range * getRandom(100))/100);

If range is more than Integer.MAX_VALUE/getRandom(100) then the
multiplication can overflow, sometimes producing a negative product.

How about something more like

return low+uniform(range);

with

/**
* Returns an integer selected with a uniform
* probability from 0..limit-1.
*/
public int uniform(final int limit) {
if (limit<=0) {
throw new IllegalArgumentException(limit+"<=0");
}
final int mini=Integer.MAX_VALUE%limit;
int candidate;
do {
candidate=random.nextInt() & Integer.MAX_VALUE;
} while (candidate<=mini);
return candidate%limit;
}

"Note that if the argument is equal to the value of
Integer.MIN_VALUE, the most negative representable int value, the result
is that same value, which is negative." -- Java API Documentation for
Math.abs

}
}

--Mike Amling

 

[Joshua Cranmer]

candidate=random.nextInt() & Integer.MAX_VALUE;

random.next(31) is arguably better for this.

 

[Mike Amling]

random.next(31) is arguably better for this.

If it had public access, I would use it. Random.next has protected
access, and the OP did not subclass Random.

--Mike Amling

 

[Daniel Pitts]

My problem is sometimes the getRandom(int low, int high) function
returns negative values.
Please review this code


import java.util.Random;

public class RNG
{
private Random random;

public RNG() {
if(random == null){

This is always true, the if is not needed.

random = new Random(System.currentTimeMillis());
}
random.setSeed(System.currentTimeMillis());

You already set the seed in the constructor. As a matter of fact, Random
does that automatically.

}

public void setSeed(long l) {
random.setSeed(l);
}

public int getRandom(int max) {
return (max == 0) ? 0 : (Math.abs(random.nextInt() % max));

There is a subtle problem with using % to cap the limit of your random
return value. Use random.nextInt(max), which does what you want.

}

///get random in range int the closed range [low, mid]
public int getRandom(int low, int high) {
if(low > high) {
int t = low;
low = high;
high = t;
}else if(low == high) {
return low;
}
int range = (high - low) + 1;
return low + ((range * getRandom(100))/100);

Your current implementation will fail at being usefully random if range
is > 100, because you can only get 100 discting random values.
Use low + random.nextInt(range);

 

[Roedy Green]

///get random in range int the closed range [low, mid]

for working code to do that, see
http://mindprod.com/jgloss/pseudorandom.html#LOWHIGH

It is much simpler than what you are attempting.

 

[markspace]

My problem is sometimes the getRandom(int low, int high) function
returns negative values.
Please review this code


import java.util.Random;

public class RNG
{
private Random random;

public RNG() {
if(random == null){
random = new Random(System.currentTimeMillis());
}
random.setSeed(System.currentTimeMillis());

As others have mentioned, the Random class already does this, so why
make another class? I'd just use some static methods.

public class MyMathUtils {

// makes this class static only, final
private MyMathUtils() {}

// Thread-safe lazy initialization
private static class LazyRandomHolder {
static final Random INSTANCE = new Random();
}

public static int getRandom( int max ) {
return LazyRandomHolder.INSTANCE.nextInt( max );
}

public static int getRandom( int a, int b ) {
if( a > b ) {
int temp = a;
a = b;
b = temp;
} else if( a == b )
return a;
long range = (long)b - a + 1;
return a + (int)(LazyRandomHolder.INSTANCE.nextDouble()*range);
}
}

Not tested!

 

[Roedy Green]

for working code to do that, see
http://mindprod.com/jgloss/pseudorandom.html#LOWHIGH

It is much simpler than what you are attempting.

As a general rule, almost any random generator code you come up with
you think would work will fail in some corner case. It is very
difficult to debug random number generator code. You can't tell easily
if it is working.

So I have two pieces of advice.

1. Always use the highest level tools available to you to solve a
given problem with the JDK.

2. Read http://mindprod.com/jgloss/pseudorandom.html
so you will be aware of just how dicey this is.

 

[Lew]

As others have mentioned, the Random class already does this, so why
make another class?  I'd just use some static methods.

public class MyMathUtils {

   // makes this class static only, final
   private MyMathUtils() {}

   // Thread-safe lazy initialization
   private static class LazyRandomHolder {
     static final Random INSTANCE = new Random();
   }

Lazy initialization is totally unnecessary. Just declare INSTANCE as
a direct member of 'MyMathUtils'.

Either way, INSTANCE is only thread safe if Random itself is thread
safe.

   public static int getRandom( int max ) {
     return LazyRandomHolder.INSTANCE.nextInt( max );
   }

....

 

[Lew]

If it had public access, I would use it. Random.next has protected
access, and the OP did not subclass Random.

private static final Random NONNEGAT =
new Random()
{
@Override public int nextInt()
{
return next(31);
}
};

 

[Mike Amling]

[...]
/**
* Returns an integer selected with a uniform
* probability from 0..limit-1.
*/
public int uniform(final int limit) {
if (limit<=0) {
throw new IllegalArgumentException(limit+"<=0");
}
final int mini=Integer.MAX_VALUE%limit;
int candidate;
do {
candidate=random.nextInt() & Integer.MAX_VALUE;
} while (candidate<=mini);
return candidate%limit;
}

Since you're already using a java.util.Random object, why
bother writing this method? What am I missing?

Good point. I agree nextInt(limit) is a better choice for practically
everyone. The code I posted is from an already old set of utilities
written so they could also run on one of our oldest machines whose
operating system is not going to be upgraded and on which the Java is
forever stuck at 1.1.7. I guess code reuse has its limits.

--Mike Amling

 

[Arne Vajhøj]

public int getRandom(int max) {
return (max == 0) ? 0 : (Math.abs(random.nextInt() % max));
}

That code is very, very wrong for getting a uniform random number in the
range [0, max]. Which is why it's generally not a good idea to reinvent
something already part of the core Java API. If you need to know why,
find 2^31 % 3.

And, FWIW, you'll also run into the problem with lower-bit short cycles
that arise out of linear congruential pseudorandom number generators.

It is unnecessary bad, because the "badness" is easy avoidable.

How bad it is will depend on how big max is and whether max is
a power of 2.

Arne

 

[guptan]

guptanwrote:

That code is very, very wrong for getting a uniform random number in the
range [0, max]. Which is why it's generally not a good idea to reinvent
something already part of the core Java API. If you need to know why,
find 2^31 % 3.

And, FWIW, you'll also run into the problem with lower-bit short cycles
that arise out of linear congruential pseudorandom number generators.

It is unnecessary bad, because the "badness" is easy avoidable.

How bad it is will depend on how big max is and whether max is
a power of 2.

Arne

Thank you all,
Originally this code is targeted for CLDC1.0. so i couldn't use
floating point numbers.

I noted a behavior that when make random instance as static to use RNG
as a singleton random property of the class becomes predictable.

 

[John B. Matthews]

[...]

I noted a behavior that when make random instance as static to use RNG
as a singleton random property of the class becomes predictable.

Using this declaration,

private static final Random random = new Random();

I do not get predictable sequences unless I invoke setSeed(long) or
instantiate Random(long seed) with the same seed. Can you provide an
example of the effect you mention?

<http://java.sun.com/javase/6/docs/api/java/util/Random.html>

 

[guptan]

I do not get predictable sequences unless I invoke setSeed(long) or
instantiate Random(long seed) with the same seed. Can you provide an
example of the effect you mention?

You are right. I had avoided setting seed inside getRandom() to reduce
computational cost.
Thanks

 

[Lew]

You are right. I had avoided setting seed inside getRandom() to reduce
computational cost.

I'm confused. Do you want the sequence to be predictable or not predictable?

The computational cost of setting the seed is negligible and irrelevant.

 

[guptan]

I was writing a non predictable random number generator for cldc1.0
.. I never tried to replace any existing functionality.
Only the following methods were available to me.

Random()
Random(long seed)
protected int next(int bits)
int nextInt()
long nextLong()
void setSeed(long seed)


I have modified the source according to your guidelines,
now reduced the bits of rng to 24 bits.


class RNG
{
private Random random;

public RNG() {
random = new Random(System.currentTimeMillis());
}

public void setSeed(long l) {
random.setSeed(l);
}

public int getRandom(int max){
return Math.abs(random.nextInt(24) % max);
}

public int getRandom(int low, int high) {
return getRandom(high - low + 1) + low;
}
}

 

[Joshua Cranmer]

public int getRandom(int max){
return Math.abs(random.nextInt(24) % max);
}

/me hits head on desk

This code works as expected if and only if max is a power of two. If it
doesn't, not every possibility is equally likely.