current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite

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>
share|improve this question
2  
The biggest improvement would be at the design level: generate passwords that should be pronounceable so they're easier to remember. Just for example, always create some number of "syllables", each of which is a consonant/vowel/consonant sequence. –  Jerry Coffin Aug 25 '13 at 15:08
    
Obvious reminder: xkcd: Password Strength –  kleinfreund Feb 9 at 15:55
    
See also Bobby's answer here –  Simon André Forsberg Feb 9 at 16:01
    
Post rolled back. Please do not update the original code after answers are posted. For a follow-up review, post a new question. –  Jamal♦ Feb 11 at 19:44
1  
See Javascript to generate random password with specific number of letters on stackoverflow. –  CodesInChaos Feb 12 at 10:00
add comment

3 Answers

up vote 8 down vote

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.

share|improve this answer
    
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? ;) –  Maximilian Fuchs Feb 8 at 16:27
add comment
up vote 5 down vote

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));
   }

    [...]

}());
share|improve this answer
add comment
up vote 3 down vote

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/

share|improve this answer
    
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. –  kleinfreund Feb 9 at 15:57
    
@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...) –  Simon André Forsberg Feb 11 at 20:09
1  
@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. –  kleinfreund Feb 11 at 20:13
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.