I see at least one big problem in your method: there isno separation of concerns. Everything is mixed into this method: common rule for password double-checking, business rules for password validation and UI calls for user response.

Extracting the business logic

The business logic in your case if the password validation specificrules, as you named themrule1,rule2a,rule2b,rule2c andrule2d.Everytime you will need a password validation, you will need these rules to validate a password, therefore I would extract these rules into a BO.

import java.util.function.Predicate;import java.util.stream.Stream;public final class Password {    private final String value;    private final Predicate<String> rule;    public Password(String value) {        this.value = value;        Predicate<String> rule1 = s -> s.length() >= 8 && s.length() <= 50;        Predicate<String> rule2a = s -> !s.equals(s.toLowerCase());        Predicate<String> rule2b = s -> !s.equals(s.toUpperCase());        Predicate<String> rule2c = s -> s.codePoints().anyMatch(Character::isDigit);        Predicate<String> rule2d = s -> s.codePoints().anyMatch(i -> !Character.isAlphabetic(i));        Predicate<String> rule2 = s -> Stream.of(rule2a, rule2b, rule2c, rule2d)                                             .filter(p -> p.test(s))                                             .count() >= 3;        this.rule = rule1.and(rule2);    }    public boolean isValid() {        return rule.test(value);    }}

If the rules are likely to change, it would also be possible to refactor this code and take the "final predicate" as a constructor argument.

Extracting "standard" logic

In your case, it isonly the rule that assert the password and the confirmation password are equals. This is not a business-specific rule but rather a commonly accepted rule everytime a user creates a password. Moreover it seems you have a "validation flow" defined (first check the equality between the two password, then check if the password in itself is valid) that can be extracted into a separate class

public final class PasswordInteractor {    public ValidationResult validatePassword(String pwd, String confirmPwd) {        if (!pwd.equals(confirmPwd)) {            return new ValidationResult("Passwords must match.", false);        }        if (!new Password(pwd).isValid()) {            return new ValidationResult("Password does not follow instructions above.", false);        }        return new ValidationResult("Password set successfully", true);    }}

I created a DTOValidationResult to transmit to the upper layer a composite result since it can be more thantrue/false.

public final class ValidationResult {    private final String message;    private final boolean success;    public ValidationResult(String message, boolean success) {        this.message = message;        this.success = success;    }    public String getMessage() {        return message;    }    public boolean isSuccess() {        return success;    }}

Extracting UI calls

Finally the UI calls must stay in the UI-part, namely into the JavaFX controller. Typically it could looks like this (simplified):

import javafx.fxml.FXML;import javafx.scene.control.Label;import javafx.scene.control.TextField;import javafx.scene.paint.Color;public final class PasswordController {    @FXML    private Label responseLabel;    @FXML    private TextField pwd;    @FXML    private TextField confirmPwd;    public void validateForm() {        // validate other parts of the form        ValidationResult result = new PasswordInteractor().validatePassword(pwd.getText(), confirmPwd.getText());        if (result.isSuccess()) {            responseLabel.setTextFill(Color.GREEN);        } else {            responseLabel.setTextFill(Color.RED);        }        responseLabel.setText(result.getMessage());    }}

• \$\begingroup\$Wow, this is extremely insightful. Thank you so much!\$\endgroup\$

  skex

  – skex

  2016-11-14 22:53:38 +00:00

  CommentedNov 14, 2016 at 22:53

Start writing some utility classes, you're likely going to be checking if strings are nonempty in other places:

public final class StringUtils {    public static boolean isNullOrEmpty(String string) {        return string == null || string.length() == 0;    }    public static boolean isNullOrWhitespace(String string) {        if (string == null) {            return true;        }        for (int i = 0; i < string.length(); i++) {            if (!Character.isWhitespace(string.charAt(i))) {                return false;            }        }        return true;    }}

You can try to generalize your password rules with an interface. A very simple approach could be just method to determine if the rule is satisfied:

public interface PasswordRule {    boolean isSatisfied(String password);}

And then you can implement some of your rules:

public class HasDigitRule implements PasswordRule {    @Override    public boolean isSatisfied(String password) {        if(password == null) throw new IllegalArgumentException("password cannot be null.");        for(int i = 0; i < password.length(); i++) {            if(Character.isDigit(password.charAt(i))) {                return true;            }        }        return false;    }}public class HasLowerCaseRule implements PasswordRule {    @Override    public boolean isSatisfied(String password) {        if(password == null) throw new IllegalArgumentException("password cannot be null.");        for(int i = 0; i < password.length(); i++) {            if(Character.isLowerCase(password.charAt(i))) {                return true;            }        }        return false;    }}public class HasNonAlphanumericRule implements PasswordRule {    @Override    public boolean isSatisfied(String password) {        if(password == null) throw new IllegalArgumentException("password cannot be null.");        for(int i = 0; i < password.length(); i++) {            if(!Character.isLetterOrDigit(password.charAt(i))) {                return true;            }        }        return false;    }}public class HasUpperCaseRule implements PasswordRule {    @Override    public boolean isSatisfied(String password) {        if(password == null) throw new IllegalArgumentException("password cannot be null.");        for(int i = 0; i < password.length(); i++) {            if(Character.isUpperCase(password.charAt(i))) {                return true;            }        }        return false;    }}public class LengthRule implements PasswordRule {    @Override    public boolean isSatisfied(String password) {        if(password == null) throw new IllegalArgumentException("password cannot be null.");        return password.length() >= 8 && password.length() <= 50;    }}

Now you can restructure you validation method to check all the critical cases first and exit early. Then count the amount of requirements met:

public boolean validatePassword(String newPassword, String confirmPassword) {    if (StringUtils.isNullOrEmpty(newPassword)            || StringUtils.isNullOrEmpty(confirmPassword)) {        responseLabel.setText("");        return false;    }    if (!newPassword.equals(confirmPassword)) {        responseLabel.setText("Passwords must match.");        responseLabel.setTextFill(Color.RED);        return false;    }    PasswordRule lengthRule = new LengthRule();    if (!lengthRule.isSatisfied(newPassword)) {        responseLabel.setText("Passwords must be between 8 and 50 characters long.");        responseLabel.setTextFill(Color.RED);        return false;    }    PasswordRule[] additionalRules = {            new HasDigitRule(),            new HasLowerCaseRule(),            new HasNonAlphanumericRule(),            new HasUpperCaseRule(),    };    long satisfiedRulesCount =            Stream.of(additionalRules)                    .filter(r -> r.isSatisfied(newPassword))                    .count();    if (satisfiedRulesCount < 3) {        responseLabel.setText("Password does not follow instructions above.");        responseLabel.setTextFill(Color.RED);        return false;    }    responseLabel.setText("Password set successfully.");    responseLabel.setTextFill(Color.GREEN);    return true;}

Going forward you should consider extracting the error messages, maybe to thePasswordRule interface. Then you could perhaps return a list of unsatisfied conditions instead of a boolean.

• \$\begingroup\$Great advice! I definitely agree about the utility classes and have now implemented that. Thank you!!!\$\endgroup\$

  skex

  – skex

  2016-11-14 22:55:22 +00:00

  CommentedNov 14, 2016 at 22:55

• java
• strings
• security
• validation
• javafx