1
\$\begingroup\$

My code generates 9 random characters, grouping them into 3 letter groups from an array of permissible characters in the form:

A4b-c56-7de

Ie, 9 upper and lower letters/numbers, separated in groups of 3 with a '-' in between. However, since I need this for other generic stuff later on, the function accepts as arguments, the number of letters (eg 9) and the number in the group (eg 3).

function CreateMyRandomId(numLetters, numInGroup){
    // Returns a string
}

I have used array manipulating functions for the first time.

  1. I use the Comma Operator so I can have a single line of code to make use of the auto return.
  2. I chain the array functions.
  3. I generate my permissible characters by array functions and the use of base 36 in toString.

I am reasonably happy, but not quite. I would also like any constructive criticism, so I can become better.

I generate an array of the permissible characters, excluding 'o', 'O' and '0'. This code will need to be typed in by the user, so characters need to be easy to differentiate from each other.

Here is how I generate my array of permissible characters:

// [0-9a-zA-Z] but excluding o, 0 and O that look the same
var myChars = [... Array(10+2*26)]
            .map((_, idx) => {
                var bl, ret;
                return (bl = idx >= 36, ret = (idx - (bl ? 26 : 0)).toString(36), bl && (ret = ret.toUpperCase()), ret);
            })
            .filter(val => !["o","O","0"].includes(val));

I do not like the map here. I wanted the auto-return, ie where there is only one statement, so you do not need to write the return. I tried to use the comma operator where The last thing will be the thing returned. However, you cannot appear to declare variables, bl and ret within the comma operator. That means you have to write a second line, which means you do not get the auto return.

You can obviously introduce a second map into the code to do the upper case. Is this good, is this bad? As a hobby programmer, I do not have anyone to say which is better.

I could do:

// [0-9a-zA-Z] but excluding o, 0 and O that look the same
var myChars = [... Array(10+2*26)]
            .map((_, idx) => (idx - (idx >= 36 ? 26 : 0)).toString(36))
            .map((val, idx) => idx >= 36 ? val.toUpperCase() : val)
            .filter(val => !["o","O","0"].includes(val));

This obviously looks a million times nicer and is easy for a third-party to read and understand. But is it as fast, is it better code?

As for the random string generator, I found that a lot easier to write, again using the comma operator for the auto return:

function CreateMyRandomId(numLetters, numInGroup) {
    return [... Array(numLetters)]
            .map(() => myChars[(Math.random() * myChars.length) | 0])
            .reduce((endArr, curVal, curIdx /*, arr not needed*/) =>
                // Left most thing in (, ) is what is returned
                (endArr[(curIdx / numInGroup) | 0].push(curVal), endArr),
            //[... Array(Math.ceil(numLetters / numInGroup))].fill([]))
            [... Array(Math.ceil(numLetters / numInGroup))].map(() => []))
            .map(curMiniArr => curMiniArr.join(''))
            .join('-');
}

I want an alternative to Math.ceil.

Math.floor can be replicated for (max bitwise safe range) floats by doing myFloat | 0.

There must be something that can be done for ceil. (I have had an idea, maybe -(~myFloat), or something on those lines. I am going to try to think of something later.

I had to create the mini arrays holding 3 letters each. I set those up at the start, and have left my mistake in (hashed out to remind me later). When I did [... Array(Math.ceil(numLetters / numInGroup))].fill([]) that used the same array for each of the mini arrays, which was wrong. I had to do [... Array(Math.ceil(numLetters / numInGroup))].map(() => []) instead to get unique mini arrays.

The function is run with:

var str = CreateMyRandomId(9, 3);

Again, any criticism of this would be much appreciated. I want to get better. I aspire to be as good as all the people on here who answer these type of questions. I aspire to be a guy who answers these questions for others.

\$\endgroup\$
1
  • \$\begingroup\$ Yes, it's ok to use .map() twice in a row - there are many instances where it can greatly improve readability to do so. Some people might hesitate because of performance, but it really doesn't hurt performance very much, and in performance-critical places, for loops should be used instead. \$\endgroup\$ May 17 at 19:39
4
\$\begingroup\$

Functions

Programmers are lazy. So much so that most of what makes a modern language is all about doing more with less effort.

The important concept to understand is the use of functions.

  • Functions let you break complex problems into smaller manageable parts.
  • Functions are reusable within the current project, and in future projects.
  • Functions are simple and do one thing only.
  • Functions save you hours of repeated coding and testing.

Too complex

Your function CreateMyRandomId is doing way to much and should be broken down into smaller functions.

Simple focused functions

Breaking the problem down into simpler parts will result in functions that are very robust and useful. The first two I use in almost every project I create.

To get a random item from an array or string

const randItem = (arr) => arr[Math.random() * arr.length | 0];

To create a set of n items

const setOf = (n, cb) => new Array(n).fill().map((v, i) => cb(i));

A function to join a set of random array items. Most of the task is solved in the above two functions.

const randStrOf = (n, arr, joiner = "") => setOf(n, () => randItem(arr)).join(joiner);

The focused result

With these three functions you can then create your higher level function with the minimum of complexity as it is only concerned with the unique parts of the problem it solves.

const createId = (n, size, chars) => setOf(n, () => randStrOf(size, chars)).join("-");

Demo

const randItem = (arr) => arr[Math.random() * arr.length | 0];
const setOf = (n, cb) => new Array(n).fill().map((v, i) => cb(i));
const randStrOf = (n, arr, joiner = "") => setOf(n, () => randItem(arr)).join(joiner);

const ALPHAS = "abcdefghijklmnpqrstuvwxyzABCDEFGHIJKLMNPQRSTUVWXYZ123456789";
const createId = (n, size, chars) => setOf(n, () => randStrOf(size, chars)).join("-");
    
console.log(createId(3, 3, ALPHAS ));

\$\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.