10
\$\begingroup\$

I wrote a simple JS app to generate passwords. I read up on some of the commenting standards for JS, and started reading the book Clean Code. I am not sure if I am writing the return statements correctly for some of my conditional test functions (e.g. hasAtLeastOne). I also tried to make my variable names semantic. Another thing I am worried about is my main function. Is it trying to do too much? I am not too worried about the HTML and CSS. They are just provided in case you want to test out the app.

// get the DOM element we will be using for the final output
var output = document.getElementById("output");

/**
 *  Main function that generates passwords after user clicks a button
 *  @return
 *      The last password
 */
function generatePasswords() {

    clearPasswords(output);

    // get DOM form elements (user input)
    var passwordAmount = document.getElementById("amount").value;
    var letterAmount = document.getElementById("letters").value;
    var numberAmount = document.getElementById("numbers").value;
    var symbolAmount = document.getElementById("symbols").value;
    letterAmount = Number(letterAmount);
    numberAmount = Number(numberAmount);
    symbolAmount = Number(symbolAmount);

    // populate character sets
    var letterSet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
    var numberSet = "0123456789";
    var symbolSet = "~!@#$%^&*()-_+=";
    var array = [];

    // character set lengths
    var letterSetLength = 52;
    var numberSetLength = 10;
    var symbolSetLength = 15;

    var passwordLength = letterAmount + numberAmount + symbolAmount;

    if(!isNumber(passwordAmount) || passwordAmount == 0) {
        alert('Password amount must be a nonzero positive number');
        return;
    } else if(passwordAmountIsTooLarge(passwordAmount)) {
        return;
    } else if(passwordLengthIsTooLong(passwordLength)) {
        return;
    } else if(hasAtLeastOne(letterAmount, numberAmount, symbolAmount)) {
        return;
    }

    for(var j = 0; j < passwordAmount; j++) {

        // random character sets initialized... to be concatenated later
        var randomLetters = "";
        var randomNumbers = "";
        var randomSymbols = "";
        var password = "";

        // generate random character subsets for password
        randomLetters = getRandoms(letterSet, letterAmount, letterSetLength, randomLetters);
        randomNumbers = getRandoms(numberSet, numberAmount, numberSetLength, randomNumbers);
        randomSymbols = getRandoms(symbolSet, symbolAmount, symbolSetLength, randomSymbols);

        password = randomLetters + randomNumbers + randomSymbols; // string concatentation
        password = password.split(''); // transform string into an array

        // shuffling
        shuffle(password);

        // transform the array into a string
        password = password.join("");

        array[j] = password; // for logging and debugging purposes

        output.innerHTML += (password + "\<br \/\>");
    }

    return array; // not useful?
}

/**
 *  Clear all previous password output so that the page doesn't overflow with
 *  old data
 *  @param {DOM element} output
 *      The DOM element to be cleared
 */
function clearPasswords(output) {

    while(output.hasChildNodes()) {
        output.removeChild(output.firstChild);
    }
}

/**
 *  Tests if the password has at least one of each type of character
 *  @param {Number} letterAmount
 *      Number of letters
 *  @param {Number} numberAmount
 *      Number of numbers
 *  @param {Number} symbolAmount
 *      Number of symbols
 *  @return
 *      True if the password has one of each character type, false otherwise
 */
function hasAtLeastOne(letterAmount, numberAmount, symbolAmount) {

    if((letterAmount < 1) || (numberAmount < 1) || (symbolAmount < 1)) {
        alert('Must have at least 1 letter, 1 number, and 1 symbol');
        return true;
    } else {
        return false;
    }
}

/**
 *  Tests if password is too logging
 *  @param {Number} passwordLength
 *      The length of the password
 *  @return
 *      True if the password is too long, false otherwise
 */
function passwordLengthIsTooLong(passwordLength) {

    if(passwordLength > 128) {
        alert('Password length cannot exceed 128 characters');
        return true;
    } else {
        return false;
    }
}

/**
 *  Tests if a number is too large
 *  @param {Number} amount
 *      Number of passwords or characters
 *  @return
 *      True if password is too large, false otherwise
 */
function passwordAmountIsTooLarge(amount) {

    if(amount > 1000) {
        alert('Cannot exceed 1000 passwords');
        return true;
    } else {
        return false;
    }
}

/**
 *  Tests if an object is a number
 *  @param {Object} object
 *      The object being tested
 *  @return
 *      True if object is a number, false otherwise
 */
function isNumber(object) {

    return !isNaN(parseFloat(object));
}

/**
 *  Randomly shuffles the character order of a given array
 *  @param {Array} password
 *      The password being shuffled
 */
function shuffle(password) {

    for(var i = 0; i < password.length; i++) {
        var randomIndex = Math.floor(Math.random() * password.length);
        var temporaryValue = password[i];
        password[i] = password[randomIndex];
        password[randomIndex] = temporaryValue;
    }
}

/**
 *  Generates a random character subset
 *  @param {String} characterSet
 *      The set of characters being chosen from at random
 *  @param {Number} characterAmount
 *      The number of characters to be chosen at random
 *  @param {Number} characterSetLength
 *      The length of the character set
 *  @param {String} output
 *      The DOM element, output
 *  @return
 *      The value of the random character subset, output
 */
function getRandoms(characterSet, characterAmount, characterSetLength, output) {

    for(var i = 0; i < characterAmount; i++) {
        var randomIndex = Math.floor((Math.random() * characterSetLength));
        output += characterSet[randomIndex];
    }

    return output;
}
* {
    /*border: 1px solid red !important;*/
    -webkit-box-sizing: border-box;
    -moz-box-sizing: border-box;
    -ms-box-sizing: border-box;
    box-sizing: border-box;
    font-family: 'Lato', sans-serif;
}

@media only screen and (max-width: 300px) {
    p {
        background-color: blue;
        color: white;
    }
}

#output {
    font-family: 'Roboto Mono';
}

.bigp {
    /*margin: 10px;*/
    font-family: sans-serif;
    font-size: 20px;
}

.right {
    text-align: right;
}

.center {
    text-align: center;
}

.grid {
    margin: 0 auto;
    max-width: 1200px;
    width: 100%;
}

.row {
    width: 100%;
    margin-bottom: 20px;
    display: flex;
    flex-wrap: wrap;
}

.col1 {
    width: 8.33%;
}

.col2 {
    width: 16.66%
}

.col3 {
    width: 25%;
}

.col4 {
    width: 33.33%;
}

.col5 {
    width: 41.66%;
}

.col6 {
    width: 50%;
}

.col7 {
    width: 58.33%;
}

.col8 {
    width: 64.66%;
}

.col9 {
    width: 75%;
}

.col10 {
    width: 83.33%;
}

.col11 {
    width: 91.66%;
}

.col12 {
    width: 100%;
}
<!DOCTYPE html>
<html>
<head>
    <title>PassGen - Random Password Generator</title>
    <!--link rel="stylesheet" src="//normalize-css.googlecode.com/svn/trunk/normalize.css"-->
    <link href="css/main.css" rel="stylesheet" type="text/css">
    <link href='http://fonts.googleapis.com/css?family=Roboto+Mono' rel='stylesheet' type='text/css'>
    <!--script src="../app/js/jquery-2.1.4.js"></script-->
</head>
<body>
    <form>
        <div class="row center">
            <div class="col3">
                <h2>Password amount</h2>
                <input type="text" id="amount" name="amount" />
            </div>
            <div class="col3">
                <h2>Letter amount</h2>
                <input type="text" id="letters" name="letters" />
            </div>
            <div class="col3">
                <h2>Number amount </h2>
                <input type="text" id="numbers" />
            </div>
            <div class="col3">
                <h2>Symbol amount</h2>
                <input type="text" id="symbols" />
            </div>
            <div class="row">
                <input onclick="generatePasswords(); return false;" type="submit" value="Generate" />
            </div>
        </div>
    </form>
    <div class="row center">
        <div class="col-12">
            <p id="output"></p>
        </div>
    </div>
    <script src="../app/plain-app.js"></script>
</body>
</html>

\$\endgroup\$
  • 2
    \$\begingroup\$ Just wanted to note that getRandoms is probably not a secure way to generate random data. Depending on the underlying implementation and your usecase it might be next to trivial to guess the state of the generator (ie the next passwords it will generate). If your unsure about that, ask a question on security.stackexchange.com \$\endgroup\$ – fho Jul 21 '15 at 8:13
  • \$\begingroup\$ @fho it certainly is not a way anyone should ever generate a password because you can predict future passwords generated by that function with extreme ease. Hence why the Web Crypto API is a thing. \$\endgroup\$ – Amelia Jul 21 '15 at 8:51
  • \$\begingroup\$ @Amelia I prefer dices :p \$\endgroup\$ – fho Jul 21 '15 at 12:37
  • \$\begingroup\$ Could you perhaps give us a screenshot of the program in action? \$\endgroup\$ – Vishvak Seenichamy Jul 21 '15 at 13:16
  • \$\begingroup\$ @VishvakSeenichamy did you not see the Run code snippet button? \$\endgroup\$ – njzk2 Jul 21 '15 at 15:15
11
\$\begingroup\$

Use built-in functions to generate random letters/numbers rather than coding by hand

Rather than list out all the letters in all combos, why not make better use of regular expressions built into javascript to more quickly and succinctly generate a random string of letters/numbers? You can see examples in this SO post: https://stackoverflow.com/questions/1349404/generate-a-string-of-5-random-characters-in-javascript

Don't pass meaningless parameters to your functions

You pass and then return an output object to GetRandom, but you only pass it an empty string. Why not just return an output object from GetRandom? Similarly, isn't the characterSetLength argument redundant when you can just use characterSet.length()? This will pass fewer parameters and save you from coding errors, where perhaps you pass in the wrong length, thus biasing your selection process (since you won't be selecting from the full range).

More error checking You do some error checking on the sum of your parameters, but shouldn't you check these initial inputs?

letterAmount = Number(letterAmount);
numberAmount = Number(numberAmount);
symbolAmount = Number(symbolAmount);

What if they are nonsense to begin with?

Make password-generation separate from for-loop for multiple passwords

Generating a single password is a distinctive task you should probably keep in its own function, separate from the task of how many passwords you want to generate. You can call this function in each iteration of your for loop.

Make use of constants

It's generally not a good idea to hard code numbers into your main coding area, as refactoring and code modification are difficult later. If you don't want a password longer than 128 characters, why not do something like

var MAX_PASSWORD_LENGTH = 128;

and then amend function passwordLengthIsTooLong(passwordLength) accordingly to use the const var rather than a normal var.

Keep logic and names consistent

Look at this function

function hasAtLeastOne(letterAmount, numberAmount, symbolAmount) {

    if((letterAmount < 1) || (numberAmount < 1) || (symbolAmount < 1)) {
        alert('Must have at least 1 letter, 1 number, and 1 symbol');
        return true;
    } else {
        return false;
    }
}

You call the function 'has at least' but you return true if the function DOES NOT have at least. Either change the name or reverse the logic.

I'm more of a generalist, so these comments are general rather than Javascript specific. Hopefully others will have more Javascript-oriented advice.

\$\endgroup\$
8
\$\begingroup\$

Don't hardcode the character set lengths. Derive them from the character sets:

// character set lengths
var letterSetLength = letterSet.length;
var numberSetLength = numberSet.length;
var symbolSetLength = symbolSet.length;

But actually, don't need these variables at all. Just use .length everywhere instead of these variables.


Don't reuse the same variable to store different kinds of values, as it can be confusing to readers. Here, password is first a string, then an array, then a string again:

    password = randomLetters + randomNumbers + randomSymbols; // string concatentation
    password = password.split(''); // transform string into an array

    // shuffling
    shuffle(password);

    // transform the array into a string
    password = password.join("");

It would be better to extract this code to a helper function shuffleLetters that shuffles the letters and returns the resulting string, so that you can use it like this:

    password = randomLetters + randomNumbers + randomSymbols;
    password = shuffleLetters(password);

This way the variable password is always one kind of type (string), and the actual shuffling logic is encapsulated outside of the main method, which is a good thing.


To check if a string is numeric, you can use isNaN directly, no need for the parseFloat call in the middle here:

function isNumber(object) {
    return !isNaN(parseFloat(object));
}

In the variable names letterAmount, numberAmount, symbolAmount, it would be better to replace "Amount" with "Count".

\$\endgroup\$
  • \$\begingroup\$ I would guess the saving of .length in a variable is caused by the belief that reading from properties takes longer time than variables. \$\endgroup\$ – Filip Haglund Jul 21 '15 at 17:40
7
\$\begingroup\$

Do not, under any circumstances, use Math.random() for a password. Ever.

Use the Web Crypto API and use RandomSource.getRandomValues().

See this question on SO for a small explanation on why Math.random is insecure (hint: it's a predictable PRNG, and you can predict future passwords in advance from your app. You can even track people via the seed).

getRandom should be something similar to this, in reality:

// not very optimized, but shows the basics of using the crypto API

function getRandomString(length, chars) {
  var crypto = window.crypto,
      buffer = crypto.getRandomValues(new Uint32Array(length)),
      password = [],
      charLength = chars.length,
      letter;
  
  for (var i = 0, j = buffer.length; i < j; i++) {
    letter = chars[buffer[i] % charLength];
    // modulus is the result were this array an infinite circle that looped,
    // allowing us to use pretty large random numbers.
    password.push(letter);
  }
  
  return password.join("");
}

var alpha = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrztuvwxyz".split("");
var numeric = "0123456789".split("");
var symbol = "!@£$%^&*()_+|\"'\\{}[]./<>?`~".split("");
var chars = alpha.concat(numeric).concat(symbol);

var password = getRandomString(50, chars);

document.getElementById("web-crypto-password").textContent = password;
<div id="web-crypto-password" style="font-family: monospace"></div>

\$\endgroup\$
  • 1
    \$\begingroup\$ For browsers that don't support the Web Crypto API, what do you suggest? MDN lists the Android browser as not supporting it at all along with no IE versions older than 11. That's not an insignificant number of users. \$\endgroup\$ – cimmanon Jul 21 '15 at 14:40
  • \$\begingroup\$ @cimmanon unfortunately if Web Crypto isn't available, then you can't have much in the way of actual security, because replacements and polyfills are written in pure javascript and subject to limitations from it, and there's no real decent CSPRNG available. Your mileage may vary; feel free to prove me wrong \$\endgroup\$ – Amelia Jul 21 '15 at 14:46
0
\$\begingroup\$

Your shuffle algorithm is unfair. See Fisher-Yates-Shuffle for a fair algorithm.

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