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('');
asked Jul 23, 2019 at 20:26
\$\endgroup\$
5
  • 2
    \$\begingroup\$ Possible duplicate of Generate random string/characters in JavaScript \$\endgroup\$ Commented Jul 23, 2019 at 20:29
  • 1
    \$\begingroup\$ Optimal in terms of readability, speed, number of characters, or something else? Also, does it need to be unguessable (secure)? \$\endgroup\$ Commented Jul 23, 2019 at 20:34
  • 5
    \$\begingroup\$ This question is too broad for StackOverflow, however is on topic for CodeReview as others have suggested. I find this to be a helpful reference in these situations: A guide to Code Review for Stack Overflow users. You'll notice that it clarifies StackOverflow questions require a specific goal (which 'more optimal' doesn't seem to reach), whereas CodeReview is tolerant of open-ended questions and answers. \$\endgroup\$ Commented Jul 23, 2019 at 20:35
  • \$\begingroup\$ Example: gitlab.com/maurygta2/fcppandjs/blob/master/Text Random/randomtext.js \$\endgroup\$ Commented Jul 23, 2019 at 23:05
  • \$\begingroup\$ en.wikipedia.org/wiki/KISS_principle \$\endgroup\$ Commented Jul 23, 2019 at 23:20

3 Answers 3

6
\$\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]}`;
}
answered Jul 23, 2019 at 22:20
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Definitely deserves a +1 for the animation. \$\endgroup\$ Commented Jul 24, 2019 at 21:35
3
\$\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)
answered Jul 23, 2019 at 21:50
\$\endgroup\$
-2
\$\begingroup\$

i'm using it to generate random characters from ranges 0–9 and a–Z :

const get_val = r => {
 // output length is 8 * r
 const t = new Float64Array(r);
 for (let n = -1; n < r; t[++n] = Math.random());
 const n = new Uint8Array(t.buffer);
 for (let r = -1; ++r < n.length; n[r] = (1 & r ? n[r] ^= n[r - 1] : n[r]) < 104 ? 65 + n[r] % 26 : 151 < n[r] ? 97 + n[r] % 26 : 48 + n[r] % 10);
 return String.fromCharCode.apply(null, n)
};
get_val(1) // GJTv8HxT
get_val(2) // 8WDAhogKJPzZrVhJ
get_val(3) // uMK5gGiItTL34W2NVIJmuUeM
get_val(4) // UOlKTicO2dKz2YdNaC2Kqc3MD4byIinD
get_val(5) // 0xHAHDuWhMTHRvpBGO0G06cOQE5q4qzRsasRS6qA
// slow...
answered Jan 28, 2022 at 12:47
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to the Code Review Community. The purpose of the code review community is to help others improve their coding skills. We do this by making insightful observations about the code posted in the question. Alternate code only solutions are considered poor answers and may be down voted or deleted by the community. If you want to make this into a good answer, please explain in the question why the code you wrote is better than the original code. \$\endgroup\$ Commented Jan 28, 2022 at 15:48

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.