CODEWITHSUNDEEP
javascript
Matt
Matt

Reputation: 1

JS Password Generator - Character length not working

I am trying to build a simple password generator in JavaScript using prompts to ask the user for password length and if they want to include certain criteria like symbols, numbers etc. However, I can't get it to return the amount of characters chosen by the user, instead, it returns just 1 character every time, rather than the amount chosen by the user.

// Assignment Code
// Generate button variable
var generateBtn = document.querySelector("#generate");

// Inputs/prompts
var passwordLength;
var symbols;
var numbers;
var uppercase;
var lowercase;

// Password character criteria
var sym = ["!", "@", "#", "$", "%", "^", "&", "*", "=", "-", "_"];
var num = ["1", "2", "3", "4", "5", "6", "7", "8", "9", "0"];
var upper = ["A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"];
var lower = ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"];


function generatePassword() {

  var password = "";
  var passwordChar = "";
  // First prompt asking for users password length
  while (true) {
    passwordLength = parseInt(prompt("Choose the length of your password. This needs to be between 8 - 128"));
    // If user chooses a number equal to or above 8 and equal to or below 128, move on
    if (passwordLength >= 8 && passwordLength <= 128) {
      break;
    }
    // Else, return this alert and loop back to the start so they can choose a valid number
    alert("Please select a number between 8 - 128");
  }
  // Once user has chosen a valid number, they will choose the rest of their criteria
  symbols = confirm("Select 'OK' if you would like to include special characters");
  numbers = confirm("Select 'OK' if you would like to include numbers");
  uppercase = confirm("Select 'OK' if you would like to include uppercase letters");
  lowercase = confirm("Select 'OK' if you would like to include lowercase letters");

  // Function to concat all possible true options and pass them to the passwordChar variable
  if (symbols) {
    passwordChar = sym;
  } else if (numbers) {
    passwordChar = num;
  } else if (uppercase) {
    passwordChar = upper;
  } else if (lowercase) {
    passwordChar = lower;
  } else if (symbols && numbers) {
    passwordChar = sym += num;
  } else if (symbols && uppercase) {
    passwordChar = sym += upper;
  } else if (symbols && lowercase) {
    passwordChar = sym += lower;
  } else if (numbers && uppercase) {
    passwordChar = num += upper;
  } else if (numbers && lowercase) {
    passwordChar = num += lower;
  } else if (uppercase && lowercase) {
    passwordChar = upper += lower;
  } else if (symbols && numbers && uppercase) {
    passwordChar = sym += num += upper;
  } else if (symbols && numbers && lowercase) {
    passwordChar = sym += num += lower;
  } else if (symbols && uppercase && lowercase) {
    passwordChar = sym += upper += lower;
  } else if (lowercase && numbers && uppercase) {
    passwordChar = lower += num += upper;
  } else if (symbols && numbers && uppercase && lowercase) {
    passwordChar = sym += num += upper += lower;
  } else if (!symbols && !numbers && !uppercase && !lowercase)
    alert("You must select at least one criterie, please start again!")

    // For loop to select random characters from the criteria strings
    for (var i = 0; i < passwordLength; i++) {
      var password = passwordChar[Math.floor(Math.random() * passwordChar.length)]
    }
  
    return (password);
}

// Write password to the #password input
function writePassword() {
  var password = generatePassword();
  var passwordText = document.querySelector("#password");

  passwordText.value = password;

}

// Add event listener to generate button
generateBtn.addEventListener("click", writePassword);
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <meta http-equiv="X-UA-Compatible" content="ie=edge" />
    <title>Password Generator</title>
    <link rel="stylesheet" href="style.css" />
  </head>
  <body>
    <div class="wrapper">
      <header>
        <h1>Password Generator</h1>
      </header>
      <div class="card">
        <div class="card-header">
          <h2>Generate a Password</h2>
        </div>
        <div class="card-body">
          <textarea
            readonly
            id="password"
            placeholder="Your Secure Password"
            aria-label="Generated Password"
          ></textarea>
        </div>
        <div class="card-footer">
          <button id="generate" class="btn">Generate Password</button>
        </div>
      </div>
    </div>
    <script src="script.js"></script>
  </body>
</html>
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <meta http-equiv="X-UA-Compatible" content="ie=edge" />
    <title>Password Generator</title>
    <link rel="stylesheet" href="style.css" />
  </head>
  <body>
    <div class="wrapper">
      <header>
        <h1>Password Generator</h1>
      </header>
      <div class="card">
        <div class="card-header">
          <h2>Generate a Password</h2>
        </div>
        <div class="card-body">
          <textarea
            readonly
            id="password"
            placeholder="Your Secure Password"
            aria-label="Generated Password"
          ></textarea>
        </div>
        <div class="card-footer">
          <button id="generate" class="btn">Generate Password</button>
        </div>
      </div>
    </div>
    <script src="script.js"></script>
  </body>
</html>

Upvotes: 0

Views: 971

Answers (5)

Hyunjune Kim
Hyunjune Kim

Reputation: 485

In your loop code, A new variable is created every time you tour the loop. That's why only one last letter is printed.

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

Change code in for loop.

// like this
let password = '';
for (let i = 0; i < passwordLength; i++) {
  password += passwordChar[Math.floor(Math.random() * passwordChar.length)]
}

In addition, you should stop using var and use let and const exclusively.

Here is why. https://evertpot.com/javascript-let-const/

Upvotes: 2

KooiInc
KooiInc

Reputation: 122916

Maybe this small factory is useful for you. This minimal reproducable example uses event delegation.

See also.

const pwdGenerator = pwdGeneratorFactory();
const handle = evt => evt.target.id === "generate" ? generatePwd() : true;
document.addEventListener(`click`, handle);

function generatePwd() {
  const len = +document.querySelector(`[type=number]`).value || 16;
  const selections = document.querySelectorAll(`[type="checkbox"]:checked`);
  const use = {}; // only lower case = default
  selections.forEach(s => use[s.value] = true);
  document.querySelector(`#result`).textContent = pwdGenerator(len, use);
}

function pwdGeneratorFactory() {
  const charDef = {
    UC: [...Array(26)].map((x, i) => String.fromCharCode(i + 65)),
    LC: [...Array(26)].map((x, i) => String.fromCharCode(i + 97)),
    Nrs: [...Array(10)].map((x, i) => i),
    Sym: `!@#$%^&*=-_;`.split(``),
    Spc: [` `],
  };
  const useDefaults = { Spc: true, UC: true, Nrs: true, Sym: true, };
  const getChars = use => Object.entries(use).reduce( (acc, [key, value]) => 
                    value ? [...acc, ...charDef[key]] : acc, charDef.LC );

  return (len = 12, use = useDefaults) => 
    shuffleRandom(getChars(use)).slice( 0, len ).join(``);
  
  // Fisher-Yates shuffle
  function shuffleRandom(array) {
      let i = array.length;
      while (i--) {
        const ri = Math.floor(Math.random() * i);
        [array[i], array[ri]] = [array[ri], array[i]];
      }
      return array;
  }
};
body {
  font: 12px/15px normal verdana, arial;
  margin: 2rem;
}

#res {
  margin-top: 0.6rem;  
}

#result {
  font-size: 1.3rem;
  font-weigth: bold;
  color: green;
  margin-left: 0.4rem;
  display: inline-block;
}
<div><input type="number" min="8" max="26" value="16"> how long?</div>
<div><input type="checkbox" value="Sym"> can I use special characters?</div>
<div><input type="checkbox" value="Nrs"> can I use numbers?</div>
<div><input type="checkbox" value="UC" checked> can I use upper case?</div>
<div><input type="checkbox" value="Spc"> can I use space?</div>
<div id="res">
  <button id="generate">Generate</button>
  <div id="result"></div>
</div>

Upvotes: 0

Swimmer F
Swimmer F

Reputation: 903

The problem is here

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

You are overwriting the passwordvariable in each iteration, therefore only returning the last letter of the password in the end. You can modify your code to add the generated letter at the end of the string like this:

    let password = '' //define empty string as a container
    for (let i = 0; i < passwordLength; i++) {
      password += passwordChar[Math.floor(Math.random() * passwordChar.length)] //add generated character to the string
    }

Upvotes: 2

eroironico
eroironico

Reputation: 1372

Since you don't need the index, you can change this:

// For loop to select random characters from the criteria strings
for (var i = 0; i < passwordLength; i++) {
    var password = passwordChar[Math.floor(Math.random() * passwordChar.length)]
}
return (password);

In:

const password = new Array(passwordLength)
                    .fill(undefined)
                    .map(c => passwordChar[Math.floor(Math.random() * passwordChar.length)]);

return password;

Upvotes: 0

Itay S
Itay S

Reputation: 73

First of all, You are using else if incorrectly.

if (symbols) {
    passwordChar = sym;
  } else if (numbers) {
    passwordChar = num;
  } else if (uppercase) {
    passwordChar = upper;
  } else if (lowercase) {
    passwordChar = lower;
  } else if (symbols && numbers) {
    passwordChar = sym += num;
  } else if (symbols && uppercase) {
    passwordChar = sym += upper;
  } else if (symbols && lowercase) {
    passwordChar = sym += lower;
  } else if (numbers && uppercase) {
    passwordChar = num += upper;
  } else if (numbers && lowercase) {
    passwordChar = num += lower;
  } else if (uppercase && lowercase) {
    passwordChar = upper += lower;
  } else if (symbols && numbers && uppercase) {
    passwordChar = sym += num += upper;
  } else if (symbols && numbers && lowercase) {
    passwordChar = sym += num += lower;
  } else if (symbols && uppercase && lowercase) {
    passwordChar = sym += upper += lower;
  } else if (lowercase && numbers && uppercase) {
    passwordChar = lower += num += upper;
  } else if (symbols && numbers && uppercase && lowercase) {
    passwordChar = sym += num += upper += lower;

If, for example, a user chose true for symbols and for numbers, your code after the first if will stop checking all the other ifs because that's how else if works. Instead do this:

const passwordArray = []
if (symbols) 
  passwordArray.concat(sym)
if (numbers) 
  passwordArray.concat(num)
if (uppercase) 
  passwordArray.concat(upper)
if (lowercase) 
  passwordArray.concat(lower)

After that you should change in your for loop to += instead of +

Upvotes: 2

Related Questions