1
\$\begingroup\$

I’m trying to generate 5 characters from the ranges 0–9 and a–Z. Is my solution optimal or did I overthink it a little? Would hardcoding a character list be more optimal?

const numbers = [
    ...Math.random()
        .toString(36)
        .substr(2, 5),
].map(element => (Math.random() > 0.5 ? element : element.toUpperCase())).join('');
\$\endgroup\$
5
\$\begingroup\$

Small Bias

First a nit pick, Math.random() generates a number from 0 to < 1. It will never generate 1. Thus to get a statistical odds of 1/2 you must test either Math.random() < 0.5 or Math.random() >= 0.5. Testing Math.random() > 0.5 ? char : char.toUpperCase() will give a very (VERY) small bias in favor of upper case characters.

Dramatic Bias

Also there is a very strong bias towards numerals in your function with a 0-9 being 2 times more likely than a-z or A-Z

The following snippet counts the occurrence of each character generated by your function and plots them (normalized) on a graph.

I animated it slowly for dramatic effect.

canvas.width = innerWidth - 10;
const ctx = canvas.getContext("2d");
const w = canvas.width;
const h = canvas.height;
const chars = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
var testCount = 2;
const val = new Array(chars.length)
val.fill(0);
const func = randString;
function randString() {
    return [
        ...Math.random()
            .toString(36)
            .substr(2, 5),
    ].map(element => (Math.random() > 0.5 ? element : element.toUpperCase())).join('');
}
function testRandomness() {
    var i = testCount;
    while (i--) {
        const a = func();
        for (const c of a) {  val[chars.indexOf(c)] += 1 }
    }
    
    const max = Math.max(...val);
    ctx.clearRect(0, 0, w, h);
    const ww = w / val.length;
    i = val.length;
    ctx.fillStyle = "blue";
    while (i--) {
        if (chars[i] === "z") { ctx.fillStyle = "green" }
        if (chars[i] === "9") { ctx.fillStyle = "red" }
        const v = val[i] / max * h;
        ctx.fillRect(i * ww, h - v, ww- 2, v)
    }
    setTimeout(testRandomness, 1000 / 30);
}
testRandomness();
canvas.addEventListener("click",()=> (testCount = 10000,d.textContent = "Sample rate ~300,000per sec") ,{once:true});
body { font-family: arial black; }
canvas {
  padding: 0px;
}
#a { color:red; }
#b { color:green; }
#c { color:blue; }
#d { font-family: arial; }
<canvas id="canvas"></canvas>
<span id="a">Red</span> 0-9 <span id="b">Green</span> a-z <span id="c">Blue</span> A-Z <span id="d">Click graph to increase sample rate to 10000</span>

Click the graph to increase the tests per sample to 10000 per 30th second (approx) and you will see that apart from the numeral bias the graph quickly becomes very flat showing no other major bias (the uppercase bias is way to small to see)

The reason for the bias is that you split the characters a-z in two when you convert half to uppercase.

Also note as pointed out in the other answer, there is a small chance that the returned string is less than 5 characters long.

Performance

In terms of performance using a lookup table is around 3 times faster ie calculate 1.5million strings in the time yours calculates 0.5million.

const chars = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
function randString(size = 5) {
    var str = "";
    while (size--) { str += chars[Math.random() * chars.length | 0] }
    return str;
}

As a function it is much more flexible, and can easily be adapted to include extra characters, or reduced character sets.

If all that matters is performance you can get around 10% faster by avoiding some of the readability introduce overhead with

function randString() {
    return  chars[Math.random() * 62 | 0] +
        chars[Math.random() * 62 | 0] +
        chars[Math.random() * 62 | 0] +
        chars[Math.random() * 62 | 0] +
        chars[Math.random() * 62 | 0];
}

Or (on chrome) the following is on average 2% faster than the one above but only after it has been run many time so that the optimizer knows what it does. If run only a few time it is slower than above

function randString3() {
    return  `${chars[Math.random()*62|0]}${chars[Math.random()*62|0]}${chars[Math.random()*62|0]}${chars[Math.random()*62|0]}${chars[Math.random()*62|0]}`;
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Definitely deserves a +1 for the animation. \$\endgroup\$ – Lee Daniel Crocker Jul 24 '19 at 21:35
2
\$\begingroup\$

There's at least one problem with this approach.

What happens if Math.random() generates a number that can be represented with just a few digits, like 0.25? You've got only one random character in that case, not five. Instead of using the fractional part of a number, maybe it would make more sense to generate integers in the range 36^4 to 36^5-1, to ensure that you have exactly 5 base-36 digits. Or use zero for the lower end of the range and pad the result with zeros, if it's important that the first character can be 0.

Also, should these be uniform? A z is only half as likely to be generated as a 9, due to only letters being transformed by uppercasing.

Aside from that, converting a string to an array and then back to a string feels a bit convoluted. You could write something like this to get the same result:

Math.random().toString(36).substr(2, 5).replace(/./g,
        m => Math.random() > 0.5 ? m.toUpperCase() : m)
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.