Random maximum bound off by 1

Troubleshooting Bug reports
1

I think random is not supposed to have that +1.

// returns a random integar between minVal and maxVal
int random(int minVal, int maxVal)
{
  // int rand(void); included by default from newlib
  return rand() % (maxVal-minVal+1) + minVal;
}

This can be an issue in code like this (from asterocks):

int rockyadd[12];
...
asterockyspeed[i]=rockyadd[random(12)]

Because of the +1, random can return 12, which is out-of-bounds for that array.

1 Like
2

AFAIR that is code straight from Arduino library, provided for your convenience and to be used with care.

1 Like
3

You mean this?

long random(long howbig)
{
  if (howbig == 0) {
    return 0;
  }
  return random() % howbig;
}

long random(long howsmall, long howbig)
{
  if (howsmall >= howbig) {
    return howsmall;
  }
  long diff = howbig - howsmall;
  return random(diff) + howsmall;
}

From WMath.cpp… it does not have this bug.

1 Like
4

Good catch. Honestly can’t remember where the +1 might have appeared. It may be from a long ways back.

Please make a pull request / issue report on github so it definitely gets fixed

2 Likes
5

That means maxVal is a terrible variable name! It implies that’ll be the maximal value returned.

howBig has the same issue too.

Edit: By the way, PHP’s rand DOES include its max value : http://php.net/manual/fr/function.rand.php

And C++'s uniform_int_distribution also does it:
http://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution

(Also a quick tip: Don’t use constants like 12, use defines (or const/constexpr) instead! e.g.

#define ROCKYADD_SIZE 12
...
int rockyadd[ROCKYADD_SIZE];
...
asterockyspeed[i]=rockyadd[random(0, ROCKYADD_SIZE - 1)];

)

6

It’s common for random number functions to use an inclusive lower bound and an exclusive upper bound because of the way the limited is implemented.
(Usually people just use the modulo (%) operator, despite the fact it introduces bias.)

The best names would be inclusiveLowerBound and exclusiveUpperBound but some people might think that’s a bit long. :P

In fact all of C++'s fancy random number classes do it, and I dare say it does make more sense to do it that way.

Unfortunately in this case the random function exists in an attempt to be compatible with Arduino, so we have to go with what they chose.

@FManga knows, he was just giving a real world example (that’s what asterocks actually does sadly).

I admire your battle against ‘magic numbers’ though.
Personally I say prefer const and constexpr const over #define because #define pollutes the whole code among other issues.

1 Like
7

I guess it’s just the name that’s wrong. I did see the exclusive upper bound somewhere haha

But that means for rolling a dice, you’d have to do random(1, 7), which sounds weird to me. Well, usages being usages, that’s OK as long as the final stuff is consistent and working, I guess!

(I do prefer const/constexpr myself, and I rely on preproc only when I can’t do something in c++ or templates haha)

1 Like
8

Yeah, that’s not my code. The magic numbers bother me less than things like i = ++i asterocks has all over the place.

Hey, it could’ve been worse. Much worse.

2 Likes
9

I know, but like I say, it’s because of how the calculation is done.
random(1, 7) translates to 1 + random() % (7 - 1) (i.e. min + random() % (max - min)).

You’re a cat after my own heart. :P

I know it was probably written by someone inexperienced, but stuff like that always hurts ; n ;.

Depends how it’s actually implemented.
I think the actual random number generation function is pre-compiled.
It’s marked extern and I’ve never found the source code for it.

1 Like