Am I writing too many if statements? can i make this code better?

Hi, i’m trying to make a password strength checker, that checks a password and says if its strong,good,fair or weak. It seems to work, though im not sure if i approached it correctly. I have a bunch of if and if else statements that seem to be difficult to read, is there any way to make it better?

my code:

function passwordValidator(password) {
  const hasUppercase = /(?=.*[A-Z])/;
  const hasDigit = /(?=.*\d)/;
  const hasSymbol = /[-!$#!@%^&*()_+|~=`{}\[\]:";'<>?,.\/]/;
  const passwordLength = [7, 12];

  if (!password) {
    return "";
  }

  if (!hasUppercase.test(password)) {
    return { name: "weak", suggestion: "Need at least one uppercase letter" };
  } else {
    if (
      (password.length >= passwordLength[1] && hasDigit.test(password)) ||
      (password.length >= passwordLength[1] && hasSymbol.test(password))
    ) {
      return { name: "strong" };
    } else if (
      password.length >= passwordLength[0] &&
      password.length <= passwordLength[1] &&
      hasDigit.test(password)
    ) {
      return { name: "good", suggestion: "Add at least one symbol" };
    } else if (
      password.length >= passwordLength[0] &&
      password.length <= passwordLength[1] &&
      hasSymbol.test(password)
    ) {
      return { name: "good", suggestion: "Add at least one digit" };
    } else if (password.length >= passwordLength[0]) {
      return { name: "fair", suggestion: "Add at least one digit or a symbol" };
    } else {
      return { name: "weak" };
    }
  }
}

Well, without even getting into the logic, anytime you have an if clause that has a return, you don’t need the else. For example:

const myFunc = x => {
  if (x > 100) {
    return "big number";
  } else if (x > 100) {
    return "small number";
  } else {
    return "average number";
  }
}

Since if the fist if is true, it will return and never hit the else. So, we can remove nesting with:

const myFunc = x => {
  if (x > 100) {
    return "big number";
  } 

  if (x > 100) {
    return "small number";
  }
  
  return "average number";
}

This will do the exact same thing, but I think is much easier to read without the nesting. you’re doing with with your first test for an empty string. I think if you applied it elsewhere, it gets more readable.

Another pattern you see sometimes is:

const myFunc = x => {
  switch (true) {
    case (x > 100) :
      return "big number";

    case (x > 100):
      return "small number";
    
    default:
      return "average number";
  }
}
1 Like

@kevinSmith thanks a lot dude, that makes a lot of sense. Switch case approach is pretty nice, but I don’t know if it’s ok when my statement is this long (password.length >= passwordLength[1] && hasDigit.test(password)) || (password.length >= passwordLength[1] && hasSymbol.test(password))
In fact it doesn’t even look good in the if statement, is it just because i’m not used to seeing that much code in the if statement or am I doing it incorrectly?

IMO switch is better option as it’s easy to read and refactor

that depends on how you organize the statements

you could do something like


if ( password.length >= passwordLength[0]) {
   if (...) {
      ...
   }
}

that would repeat that expression only once, and put all the cases for which that has to be true, inside it

also, always remember that the first true condition is the one executed, if you order them in the right way, you can reduce the length of the statements

If that gets ugly, you can always store that value in a variable before you begin the switch or use a function to return the boolean.

@kevinSmith thanks that helped me