8
\$\begingroup\$

Can you please take a look at my code and improve it (if necessary)?

http://jsfiddle.net/U6R6E/

Javascript (with jQuery)

function random(min, max) {
    return min + parseInt(Math.random() * (max - min + 1), 10);
}
function generatePassword() {
    var length = parseInt($('#pwLength').val(), 10),
        charset = $('#pwChars').val(),
        password = "";
    while (length > 0) {
        length -= 1;
        console.log(length);
        password += charset[random(0, charset.length - 1)];
    }
    return password;
}
function getNewPassword() {
    $('#pwResult').html(generatePassword());
}
$(document).ready(function () {
    getNewPassword();
    $('#getNewPw').click(function () {
        getNewPassword();
        return false;
    });
});

HTML

<ul>
    <input type="text" id="pwChars" value="AaBbCcDdEeFfGgHhiJjKkLMmNnoPpQqRrSsTtUuVvWwXxYyZz23456789!?$%#&@+-*=_.,:;()" />
    <li><input type="text" id="pwLength" value="10" /></li>
    <li><button id="getNewPw">New</button></li>
</ul>

<div id="pwResult" contenteditable="true"></div>
\$\endgroup\$
8
\$\begingroup\$

Math.random() doesn't return cryptographically secure numbers on all browsers. If this is intended for production use, you'll want to use a library that has a secure PRNG.

If you're not going to go with Jerry's suggestion to make pronounceable passwords, I'd recommend at least getting rid of 1/l/I and O/0, which a number of password generators do by default, because people often mis-read them and then request a password reset.

\$\endgroup\$
  • \$\begingroup\$ thanks! Can you write me the steps how to install the "Stanford Javascript Crypto Library" in my script or maybe do it in my JSfiddle? ;) \$\endgroup\$ – Maximilian Fuchs Feb 8 '14 at 16:27
5
\$\begingroup\$

Your code looks really good! However, since I have to nitpick...

  • I would definitely save your jQuery selectors, since they are really constants. And so that you're not making globals, I'd also create a IIFE.

  • Your random function is excellent, but parseInt is quite slow, there's a bit-twiddling trick you can use to convert a double to an int using ~~, so I've changed it to that. Or use Math.round().

Everything else I would keep the same :)

(function () {

   var $length, $result, $new, $chars;

   $(document).ready(function () {

      $length = $('#pw-length');
      $result, = $('#pw-result');
      $new = $('#get-new-pw');
      $chars = $('#pwChars');

      getNewPassword();
      $new.click(function () {
        getNewPassword();
        return false;
      });

   });

   // get password
   function random(min, max) {
     return min + ~~(Math.random() * (max - min + 1));
   }

    [...]

}());
\$\endgroup\$
3
\$\begingroup\$

Some comments on your code:

  1. CSS's naming convention is to use hyphens, so instead of pwChars you'd write pw-chars.

  2. You could simplify your code with a Fisher-Yates shuffle helper from here, then you can write your logic in a few lines, but this method requires as many distinct characters as the length needed. So if you need an 8 character password there must be at least 8 characters in the dictionary.

    $('#getNewPw').click(function () {
        $('#pwResult').text(function () {
            var chars = $('#pwChars').val().split('');
            var len = $('#pwLength').val();
            return shuffle(chars).slice(0, len).join('');
        });
    }).click();
    

    Demo: http://jsfiddle.net/U6R6E/4/

\$\endgroup\$
  • \$\begingroup\$ Just on a side note: You're talking about CSS, but there is no CSS in his code. What you're talking about are HTML classes. Also there is no official naming convention for HTML ID's and classes. \$\endgroup\$ – kleinfreund Feb 9 '14 at 15:57
  • \$\begingroup\$ @kleinfreund Even though there is no CSS in the code, HTML id's are usually used for CSS (and JavaScript, of course). And even though there might not be any official naming conventions for HTML ID's and classes, pw-chars does seem more common than pwChars. I rarely see camelcasing on HTML ids and classes. (That being said, it doesn't mean that there is necessarily anything wrong with camelcasing them...) \$\endgroup\$ – Simon Forsberg Feb 11 '14 at 20:09
  • 1
    \$\begingroup\$ @Simon This is right. Naming in HTML is usually done by dash-delimiting instead of camelcasing. Also there are rarely capital letters. I just wanted to be precise, because it's a common mistake speaking of CSS classes, which do not exist. \$\endgroup\$ – kleinfreund Feb 11 '14 at 20:13

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.