Asynchronous recursive function for generating a unique Id

Question

The code below is my first purpose built asynchronous recursive function. It took me a while to figure out how to write the thing, and I would like a second opinion. It's part of a project for The Odin Project, a simple book log. I don't have the site live yet, I want to implement a few more features first. I was hoping that you all could take a look at it and let me know what I did right and what I did wrong.

The purpose is to create a unique ID for a book when it is created, by first generating the possible ID, then checking it against the given array of books currently in play.

const randomNum = (minNum, maxNum) => {
  const min = Math.ceil(minNum);
  const max = Math.floor(maxNum);
  return Math.floor(Math.random() * (max - min) + min);
};

const letters = 'ABCDEFGHIJKLMNPRSTUVWXYZ';
const generateId = () => {
  return `${letters[randomNum(1, 24)]}${randomNum(1000, 9999)}`;
};

const makeId = async (checkedArray) => {
  const newId = generateId();

  if (!checkedArray.includes(newId)) {
    return Promise.resolve(newId);
  }

  const recursiveResult = await new Promise((resolve, reject) => {
    if (checkedArray) {
      resolve(makeId(checkedArray));
    } else {
      reject('makeId failed');
    }
  });
  return recursiveResult;
};

The biggest hang up that I ran into was where to put the successful result's return statement. I wanted to get this without asking for help, so it took me a couple of days worth of research. I finally found out about how and when to return a Promise.resolve() and with happy abandon I implemented it.

And I have to say, figuring it out myself was immensely satisfying!

One alternative that I came up with was to use the current date. But I felt that while it worked, it wasn't an ideal solution... Unless it was...

const altId = (bookTitle) => {
  return `${bookTitle.replace(/\s/g, '')}${Date.now()}`;
};

Answer 1

Over engineered

This code is over engineered.

• It is not asynchronous (nothing is pausing execution)
• The id is too complex for the domain of books (ids in checkedArray) even if that list comes from a DB
• The random function is needlessly truncating the range values.

And there is a bug in the code.

UID

If you want a GUID (global unique ID) or UUID (Universally Unique ID) unique to all IDs ever made, then it is best to follow a standard algorithm.

Note that it is possible for a duplicate ID to be created as there is no way to check it against all GUIDs. It thus relies on statistics, the odds of any ID already existing is vanishingly small.

From what I can make out from your question the domain is a database of books. The best way to ensure a unique Id for each book is for the database to store a table that holds an ID source. For each book added you query the ID and increment its value in the DB by 1.

This will ensure each new book in the DB will have a unique ID. Even for a 32bit int the number of unique IDs available will easily cover all books likely to be stored in the DB.

Async?

There is nothing in the code you have presented that requires a promise. You can return the result (an ID or the string 'makeId failed') immediately without the need to create and/or resolve promises

Bug?

There is a bug in the function makeId. See comment marks in the snippet

• /* CODE MARK B */ checks if checkedArray exists which suggest that it is possible for it not to exist. However

• /* CODE MARK A */ requires checkedArray to exist and to have the function includes

const makeId = async (checkedArray) => {
  const newId = generateId();
  if (!checkedArray.includes(newId)) {                             /* CODE MARK A */
    return Promise.resolve(newId);
  }
  const recursiveResult = await new Promise((resolve, reject) => {
    if (checkedArray) {                                            /* CODE MARK B */
      resolve(makeId(checkedArray));
    } else {
      reject('makeId failed');
    }
  });
  return recursiveResult;
};

Either...

• B is not required as there will always be an array making the guard redundant, or
• A is unsafe and will throw an error and reject the id request without the expected reject message 'makeId failed'

Either case is unexpected (or inconsistent) behavior and thus considered a BUG.

Random

I can not see the need for you to truncate the min and max values in the random function. It is unneeded complexity.

Rewrite random

The following rewrites of randomNum will always return an int.

Note that the name randomNum is poor as it returns not a number but an integer. A better name is randomInt

const randomInt = (min, max) => Math.floor(Math.random() * (max - min) + min);

// or if you only want a positive
// using a bitwize OR | to floor will return a positive 32bit int.

const randomUint32 = (min, max) => Math.random() * (max - min) + min | 0;