Please review this code
guptan
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);
}
}
bugbear
> My problem is sometimes the getRandom(int low, int high) function
> returns negative values.
> Please review this code
If low is negative, you'd expect that.
BugBear
rossum
wrote:
>My problem is sometimes the getRandom(int low, int high) function
>returns negative values.
What value was the low parameter set to?
>Please review this code
>
>
>import java.util.Random;
>
>public class RNG
>{
> private Random random;
>
> public RNG() {
> if(random == null){
> random = new Random(System.currentTimeMillis());
Why are you using lazy initialisation here? Is there ever any case
when on calling the constructor the value of random is not null?
Lazy initialisation may be useful if the object is both costly to
initialise and static. Your random is neither.
I would prefer System.nanoTime() to System.currentTimeMillis(), though
the Random constructor seeds itself automatically anyway by default.
Simpler just to say:
private Random - new Random();
and have a default constructor.
> }
> 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));
What is wrong with:
return random.nextInt(max + 1); ?
> }
>
>///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;
I would be inclined to throw an exception if low > high. That
indicates that there is some problem in the calling code which should
be flagged back to the caller with an exception.
> }else if(low == high) {
> return low;
> }
> int range = (high - low) + 1;
> return low + ((range * getRandom(100))/100);
Look at what might happen if range > Integer.MAX_VALUE / 100 This is
a possible source of your problem with negative numbers.
Better to use something like:
return low + random.nextInt(range);
> }
>}
Much of your code is reinventing the wheel of what is already present
in java.util.Random This is fine for a programming exercise, but
pointless for real work.
rossum
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.
--
Beware of bugs in the above code; I have only proved it correct, not
tried it. -- Donald E. Knuth
Andre Silaghi
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
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
> Mike Amling wrote:
>> candidate=random.nextInt() & Integer.MAX_VALUE;
>
> 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){
> random = new Random(System.currentTimeMillis());
> }
> random.setSeed(System.currentTimeMillis());
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));
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);
--
Daniel Pitts' Tech Blog: <http://virtualinfinity.net/wordpress/>
Eric Sosman
> [...]
> /**
> * 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?
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.
--
Roedy Green Canadian Mind Products
http://mindprod.com
http://thecovemovie.com : The Cove: a documentary about Japan's secret atrocities against dolphins.
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
On Wed, 19 Aug 2009 12:18:44 -0700, Roedy Green
<see_w...@mindprod.com.invalid> wrote, quoted or indirectly quoted
someone who said :
>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
Patricia Shanahan
> 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());
> }
In addition to the other comments, I suggest writing out the value of
the seed somewhere, and providing a way to proved the seed value from
outside the program.
Debug is *much* easier if you can repeat runs with the same seed, and
therefore the same sequence of Random results.
Patricia
Lew
>>> candidate=random.nextInt() & Integer.MAX_VALUE;
Joshua Cranmer wrote:
>> random.next(31) is arguably better for this.
Mike Amling wrote:
> 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);
}
};
--
Lew
Mike Amling
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
> guptan wrote:
>> 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
> Joshua Cranmer wrote:
> >guptanwrote:
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.
>guptan
John B. Matthews
<63ad6dee-43c6-43bb...@a7g2000yqo.googlegroups.com>,
guptan <man...@gmail.com> wrote:
[...]
> 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>
--
John B. Matthews
trashgod at gmail dot com
<http://sites.google.com/site/drjohnbmatthews>
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
>> 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?
guptan wrote:
> 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.
--
Lew
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.
Eric Sosman
> On 09/29/09 06:27, guptan wrote:
>> 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.
It works just fine if max is 1, 2, 3, 4, 6, 8, 12, 24 or one of
their negatives. For all other values of max it produces an uneven
distribution -- yes, even for powers of two like 16 and 1024.
(Hint: nextInt(int) is not the same as bits(int).)
John B. Matthews
<4c1d9eef-e4ee-4021...@z24g2000yqb.googlegroups.com>,
guptan <man...@gmail.com> wrote:
> class RNG
> {
> private Random random;
>
> public RNG() {
> random = new Random(System.currentTimeMillis());
> }
> ...
> }
Depending on your processor speed and clock resolution, repeated use of
this constructor may result in identically seeded generators. The
no-parameter constructor already "sets the seed of the random number
generator to a value very likely to be distinct from any other
invocation of this constructor."
random = new Random();
<http://java.sun.com/javase/6/docs/api/java/util/Random.html#Random()>
Roedy Green
wrote, quoted or indirectly quoted someone who said :
>
>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.
The main thing that jumped out at me is it is missing the JavaDoc.
Without that, I don't even know for sure what you are TRYING to do.
--
Roedy Green Canadian Mind Products
http://mindprod.com
On two occasions I have been asked [by members of Parliament], "Pray, Mr. Babbage, if you put into the machine wrong figures, will the right answers come out?" I am not able rightly to apprehend the kind of confusion of ideas that could provoke such a question.
~ Charles Babbage (born: 1791-12-26 died: 1871-10-18 at age: 79)
guptan
ok. Please get me a reliable solution for this one assuming the
limitations of j2me cldc 1.0 platform.
public class RNG extends Random
{
public RNG(long seed) {
super(seed);
}
public int getRandom(int max){
//Your suggested code here >>>>>>>>>>>>>
Lew
>> --
>> Eric.Sos...@sun.com
It is not necessary and is unconventional to quote sigs.
> ok. Please get me a reliable solution for this one assuming the
> limitations of j2me cldc 1.0 platform.
>
> public class RNG extends Random
You should compose rather than extend.
private final Random rand;
> {
> public RNG(long seed) {
> super(seed);
rand = new Random( seed );
> }
>
> public int getRandom(int max){
> //Your suggested code here >>>>>>>>>>>>>
Assuming you want an int from the range [0, max},
return rand.nextInt( max );
> }
>
> public int getRandom(int low, int high) {
> return getRandom(high - low + 1) + low;
You'll need to range-check the arguments for this formula.
> }
> }
--
Lew
Do not quote this sig.
Eric Sosman
>
> ok. Please get me a reliable solution for this one assuming the
> limitations of j2me cldc 1.0 platform.
>
> public class RNG extends Random
> {
> public RNG(long seed) {
> super(seed);
> }
>
> public int getRandom(int max){
> //Your suggested code here >>>>>>>>>>>>>
> }
>
> public int getRandom(int low, int high) {
> return getRandom(high - low + 1) + low;
> }
> }
Since you have not said what you want getRandom(int) to do, I have
nothing to suggest. The obvious thing would be to use nextInt(max), but
since you're not doing the obvious there must be some other behavior
you desire. I don't know what that desired behavior might be.
markspace
>
> ok. Please get me a reliable solution for this one assuming the
> limitations of j2me cldc 1.0 platform.
Why do you think the Random class is not reliable?