javascript - Is it a bad practice to use multiple nested If Else statements? - Stack Overflow

admin管理员组

文章数量:1426066

I'm trying to make a validator for an interface to change the password, and while making it I realized that I was using a lot of if else statements. I was wondering if there's a better way to do it.

My Code :

function changePassword() {

  var testPassword = "user";
  var oldPassword = document.getElementById('password-old').value;
  var newPassword = document.getElementById('password-new').value;
  var newPasswordRepeat = document.getElementById('password-new-repeat').value;

  if (oldPassword === testPassword) {
    if (newPassword !== newPasswordRepeat) {
      alert("The passwords don't match");
    } else {
      if (newPassword === "") {
        alert("New password can't be blank");
      } else {
        alert("Password was changed successfully");
      }
    }
  } else {
    alert("Incorrect Password")
  }
}

<label for="password-old">Old Password</label>
<input type="text" id="password-old" />

<label for="password-new">New Password</label>
<input type="text" id="password-new" />

<label for="password-new-repeat">Re-Enter New Password</label>
<input type="text" id="password-new-repeat" />

<input type="button" value="Confirm" onclick="changePassword()" />

I'm trying to make a validator for an interface to change the password, and while making it I realized that I was using a lot of if else statements. I was wondering if there's a better way to do it.

My Code :

function changePassword() {

  var testPassword = "user";
  var oldPassword = document.getElementById('password-old').value;
  var newPassword = document.getElementById('password-new').value;
  var newPasswordRepeat = document.getElementById('password-new-repeat').value;

  if (oldPassword === testPassword) {
    if (newPassword !== newPasswordRepeat) {
      alert("The passwords don't match");
    } else {
      if (newPassword === "") {
        alert("New password can't be blank");
      } else {
        alert("Password was changed successfully");
      }
    }
  } else {
    alert("Incorrect Password")
  }
}

<label for="password-old">Old Password</label>
<input type="text" id="password-old" />

<label for="password-new">New Password</label>
<input type="text" id="password-new" />

<label for="password-new-repeat">Re-Enter New Password</label>
<input type="text" id="password-new-repeat" />

<input type="button" value="Confirm" onclick="changePassword()" />

Is there a shorter way to do it or should I keep it as it is?

• javascript
• html

Share Improve this question Follow asked Jun 10, 2020 at 9:20 pokerface0958pokerface0958 8511 silver badge88 bronze badges 14

• 1 This question belongs at codereview, rather than here – mplungjan Commented Jun 10, 2020 at 9:24
• @mplungjan sorry about that, im still pretty new here so i wasnt fully aware about that site. ill use that next time :) – pokerface0958 Commented Jun 10, 2020 at 9:52
• 1 See this post on the sister site Software Engineering: How would you refactor nested IF Statements?. Note this post too is a duplicate which links to further posts on this subject. As this is a very broad and opinion-based topic, it's not a good fit for Stackoverflow. – Boaz Commented Jun 10, 2020 at 9:57
• The snippet doesn't appear to work. It shows me Incorrect Password even when I'm absolutely sure I've made a new password that's according to all rules. A proper validator would tell exactly which rule is violated, which yours doesn't. – Mast Commented Jun 10, 2020 at 10:00
• @Boaz-ReinstateMonica thanks! ill check that now – pokerface0958 Commented Jun 10, 2020 at 10:01

 |  Show 9 more ments

4 Answers 4

Sorted by: Reset to default 3

I would shortcut for readability

Note I specifically return a true or false and not a falsy or no value as in the other answers.

In OP's example the script is executed on click of a button. Using my code you can attach it to an event handler or use it in another statement that needs the result of the changePassword function. For example:

function changePassword() {

  var testPassword = "user";
  var oldPassword = document.getElementById('password-old').value;
  var newPassword = document.getElementById('password-new').value;
  var newPasswordRepeat = document.getElementById('password-new-repeat').value;

  if (newPassword === "") {
    alert("New password can't be blank");
    return false;
  }
  if (oldPassword !== testPassword) {
    alert("Incorrect Password")
    return false;
  }
  const valid = newPassword === newPasswordRepeat
  alert(valid ? "Password was changed successfully" : "The passwords don't match");
  return valid;
}

const myForm = document.getElementById("myForm");
const success = document.getElementById("success");
myForm.addEventListener("submit", (e) => {
  e.preventDefault(); // we do not submit (you could if you wanted though) 
  const passWordChanged = changePassword();
  myForm.hidden = passWordChanged; // the form will hide when successful
  success.hidden = !passWordChanged; // show (hidden = false)
  
});

.form-container {
  width: 50%; /* Adjust the width as needed */
  margin: 0 auto; /* Center the form in the page */
}

.form-row {
  display: grid;
  grid-template-columns: auto minmax(200px, 1fr);
  gap: 10px;
  margin-bottom: 10px;
}

label {
  text-align: right; /* Align text to the right for labels */
}

input[type="text"] {
  width: 100%; /* Input takes the full width of its grid area */
}

input[type="submit"] {
  grid-column: 2; /* Position the submit button under the inputs */
  margin-top: 10px;
}

<h1 id="success" hidden>Password successfully changed</h1>
<div class="form-container">
  <form id="myForm">
    <div class="form-row">
      <label for="password-old">Old Password</label>
      <input type="text" id="password-old" />
    </div>
    <div class="form-row">
      <label for="password-new">New Password</label>
      <input type="text" id="password-new" />
    </div>
    <div class="form-row">
      <label for="password-new-repeat">Re-Enter New Password</label>
      <input type="text" id="password-new-repeat" />
    </div>
    <div class="form-row">
      <input type="submit" value="Confirm" />
    </div>
  </form>
</div>

You could return early.

function changePassword() {

  var testPassword = "user";
  var oldPassword = document.getElementById('password-old').value;
  var newPassword = document.getElementById('password-new').value;
  var newPasswordRepeat = document.getElementById('password-new-repeat').value;

  if (oldPassword !== testPassword) return alert("Incorrect Password");
  if (newPassword !== newPasswordRepeat) return alert("The passwords don't match");
  if (newPassword === "") return alert("New password can't be blank");
  alert("Password was changed successfully");
}

<label for="password-old">Old Password</label>
<input type="text" id="password-old" />

<label for="password-new">New Password</label>
<input type="text" id="password-new" />

<label for="password-new-repeat">Re-Enter New Password</label>
<input type="text" id="password-new-repeat" />

<input type="button" value="Confirm" onclick="changePassword()" />

Yes. those nested if statements tend to decrease the readability. In your case, you can simply return from the method to avoid the nesting and make the code more understandable.

function changePassword() {

  var testPassword = "user";
  var oldPassword = document.getElementById('password-old').value;
  var newPassword = document.getElementById('password-new').value;
  var newPasswordRepeat = document.getElementById('password-new-repeat').value;

  if (oldPassword !== testPassword) {
    alert("Incorrect Password")
    return
  }

  if (newPassword !== newPasswordRepeat) {
    alert("The passwords don't match");
    return;
  }

  if (newPassword === "") {
    alert("New password can't be blank");
  } else {
    alert("Password was changed successfully");
  }

}

Actually multiple nested if else increases Cyclomatic plexity and thus should be avoided wherever is possible.

Cyclomatic plexity is defined as the number of linearly independent paths through the code.

So you should refactor your code to have less branches. You can rewrite your function as:

function changePassword() {

      var testPassword = "user";
      var oldPassword = document.getElementById('password-old').value;
      var newPassword = document.getElementById('password-new').value;
      var newPasswordRepeat = document.getElementById('password-new-repeat').value;

      if (oldPassword != testPassword) {  
        alert("Incorrect Password")
      }
      else if (newPassword !== newPasswordRepeat) {
        alert("The passwords don't match");
      }  
      else if (newPassword === "") {
        alert("New password can't be blank");
      }
      else {
        alert("Password was changed successfully");
      }
}

本文标签： javascriptIs it a bad practice to use multiple nested If Else statementsStack Overflow