DEV Community

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

Collapse
lukeshiru profile image
LUKESHIRU • Edited on

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;
    }
}
Enter fullscreen mode Exit fullscreen mode

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!

Collapse
nombrekeff profile image
Keff Author

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!

Collapse
lukeshiru profile image
LUKESHIRU • Edited on

My main reason is mainly usage:

import { Something } from "./Something";

console.log(Something.aValue);

// vs

import { aValue } from "./Something";

console.log(aValue);
Enter fullscreen mode Exit fullscreen mode

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

Thread Thread
nombrekeff profile image
Keff Author

Ahh okay, makes sense

Collapse
nombrekeff profile image
Keff Author

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!

Collapse
inhuofficial profile image
InHuOfficial

Just wondering, is it faster to do

  password = password.concat(combinedCaracters[this._random(0, length)]);
Enter fullscreen mode Exit fullscreen mode

vs just

  password += combinedCaracters[this._random(0, length)];
Enter fullscreen mode Exit fullscreen mode

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.

Thread Thread
nombrekeff profile image
Keff Author

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