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.

I would like to receive feedback about my code. Is there any better way to shorten the code or is it fine?

Demo

var pass_strength;
function IsEnoughLength(str,length){
    if ((str == null) || isNaN(length))
        return false;
    else if (str.length < length)
        return false;
    return true;
}
function HasMixedCase(passwd){
    if(passwd.match(/([a-z].*[A-Z])|([A-Z].*[a-z])/))
        if ( passwd.split(/[a-z]/).length >= 3 && passwd.split(/[A-Z]/).length >= 3)
            return true;
        else
            return false;
    else
        return false;
}
function HasNumeral(passwd){
    if(passwd.match(/[0-9]/))
        return true;
    else
        return false;
}
function HasSpecialChars(passwd){
    if(passwd.match(/.[!,@,#,$,%,^,&,*,?,_,~]/))
        return true;
    else
        return false;
}
function CheckPasswordStrength(pwd)
{
    if (IsEnoughLength(pwd,14) && HasMixedCase(pwd) && HasNumeral(pwd) && HasSpecialChars(pwd))
        pass_strength = "<b><font style='color:olive'>Very strong</font></b>";
    else if (IsEnoughLength(pwd,10) && HasMixedCase(pwd) && HasNumeral(pwd) && HasSpecialChars(pwd))
        pass_strength = "<b><font style='color:Blue'>Strong</font></b>";
    else if (IsEnoughLength(pwd,10) && HasMixedCase(pwd) && HasNumeral(pwd))
        pass_strength = "<b><font style='color:Green'>Medium</font></b>";
    else
        pass_strength = "<b><font style='color:red'>Weak</font></b>";
    document.getElementById('pwd_strength').innerHTML = "Password strength: " + pass_strength;
}
share|improve this question
    
It's worth noting that a password doesn't have to have mixed case, special characters or numbers to be strong - It can be strong with nothing but 4 or 5 english words. See xkcd.com/936 –  Patashu May 2 at 3:27
    
Use an existing realistic password strength estimator like zxcvbn. –  Gumbo May 11 at 6:31

2 Answers 2

  1. You can replace all of the

    if(x.match(...))
        return true;
    else
        return false;
    

    with:

    return x.match(...) !== null;
    

    or if you're content with returning a truthy/falsy value instead of proper booleans you can even simplify it to:

    return x.match(...)
    
  2. IsEnoughLength is ungrammatical. I'd use IsLongEnough

  3. You're mixing the password strength estimation with presentation logic. You should have one function which returns a value indicating the strength of the password and another which manipulates the DOM.
  4. Use CSS classes to format the text instead of <b> and <font style=
  5. Personally I wouldn't bother with argument checking in such a simple internal function. But if you do, throw an exception on programmer error, don't just return false.

    if ((str == null) || isNaN(length))
        return false;
    

    should be something like

    if (str == null)
        throw new Error("str must not be null");
    if(isNaN(length))
        throw new Error("length must be a number");
    

    (or a fancier exception class if you want)

share|improve this answer
    
Thank you very much for your reply. I will try like that :) –  Athimannil May 1 at 15:16

As @CodesInChaos has already pointed out, the boolean expressions can be simplified, a lot. For example:

function HasMixedCase(passwd) {
    return passwd.match(/([a-z].*[A-Z])|([A-Z].*[a-z])/)
        && passwd.split(/[a-z]/).length >= 3
        && passwd.split(/[A-Z]/).length >= 3;
}
function HasNumeral(passwd) {
    return passwd.match(/[0-9]/);
}
function HasSpecialChars(passwd) {
    return passwd.match(/.[!,@,#,$,%,^,&,*,?,_,~]/);
}

In addition, I would simplify the method that checks the length like this:

function IsLongEnough(passwd, length) {
    return passwd && passwd.length > length;
}

This will only return true if passwd is not undefined and long enough.

And I dropped the validation of the length parameter. For one thing, it was unnecessary: IsLongEnough(word, undefined) and IsLongEnough(word, NaN) will always return false. For another thing, if you are calling password validation methods with invalid parameters, you are in big trouble!

share|improve this answer

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.