Discussion on: Let's optimize JavaScript - password generator (2.15x faster)

[Luke Shiru]

A few more improvements that you could have (mainly for DX, not so much for perf) are:

1. Make private members actually private with #.
2. Make use of getters and setters.
3. Move the statics to constants.
4. Classic class procedure of this.generate = this.generate.bind(this);, so folks can use that function in stuff like maps without having issues.

Applying those suggestions, the code looks something like this (I added JSDocs to have better DX as well):

/**
 * @typedef PasswordGeneratorOptions
 * @property {number} [length=12]
 * @property {boolean} [lowercase=true]
 * @property {boolean} [numbers=true]
 * @property {typeof mathRandom} [randomFunction=mathRandom]
 * @property {boolean} [symbols=false]
 * @property {boolean} [uppercase=true]
 */

/**
 * @param {number} min
 * @param {number} max
 */
const mathRandom = (min, max) => Math.floor(Math.random() * (max - min) + min);

const uppercaseChars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
const lowercaseChars = "abcdefghijklmnopqrstuvwxyz";
const symbolsChars = "<>[]{}=?()&%$#@!¡?¿*_-.:;,";
const numbersString = "0123456789";

export class PasswordGeneratorFast {
    /** @type {PasswordGeneratorOptions} */
    #options = {
        length: 12,
        lowercase: true,
        numbers: true,
        randomFunction: mathRandom,
        symbols: false,
        uppercase: true
    };
    #characters = "";
    #optionsChanged = true;

    set options(options) {
        this.#optionsChanged = true;
        this.#options = {
            ...this.#options,
            ...options
        };
    }

    get options() {
        return this.#options;
    }

    get characters() {
        if (this.#optionsChanged) {
            this.#characters =
                (this.#options.lowercase ? lowercaseChars : "") +
                (this.#options.uppercase ? uppercaseChars : "") +
                (this.#options.symbols ? symbolsChars : "") +
                (this.#options.numbers ? numbersString : "");
            this.#optionsChanged = false;
        }

        return this.#characters;
    }

    /** @param {PasswordGeneratorOptions} options */
    constructor(options = {}) {
        this.options = options;
        this.generate = this.generate.bind(this);
    }

    generate() {
        const { characters } = this;
        const length = characters.length;
        let password = "";

        for (let char = 0; char < this.#options.length; char++) {
            password = password.concat(
                characters[this.#options.randomFunction(0, length)]
            );
        }

        return password;
    }
}

Performance wise mine maybe is worst (didn't tested it, but I think setters/getters don't perform as well as just using methods, I might be wrong), but besides performance, we always have to factor DX, and from my point of view, getters and setters when working with classes provide a better DX. I personally prefer to just use functions and not even go into classes, but still 🤣

Cheers!

[Keff]

I usually take DX quite seriously on more important things, this was me just having a bit of fun.

The only thing I'm not sure about your code is, the use of constants instead of the static fields on the class. My reasoning behind this was to be able to change them in case the user wanted to use cyrillic or some other alphabet. Although thinking about it now, it would've made more sense to pass them in the options. Any reason why using constants would be considered better DX in this scenario?

Cheers and thanks for the additions!

[Luke Shiru]

My main reason is mainly usage:

import { Something } from "./Something";

console.log(Something.aValue);

// vs

import { aValue } from "./Something";

console.log(aValue);

Super niche, I know, but the kind of encapsulation that used to be useful from classes now I get from modules :D

[Keff]

Ahh okay, makes sense

[Keff]

Ohh, I also tested the performance of your code, it scored second amongst the rest of the cases. So get/set did not affect significantly, so no big sacrifice there!

[GrahamTheDev]

Just wondering, is it faster to do

  password = password.concat(combinedCaracters[this._random(0, length)]);

vs just

  password += combinedCaracters[this._random(0, length)];

I would imagine as concat is not performant it would be faster.

Only glanced at it though so I might have missed a good reason for concat.

[Keff]

I though that too, but I did some benchmarks, and found out concat to perform a bit faster than +=. It was not the most extensive benchmark, and it might vary from browser to browser and such... but would be interesting to know though