Spring password validator library

Question

I recently got rejected at a job interview for submitting this:

https://bitbucket.org/gnerr/password-validator

The interviewer asked for a password validation library that was configurable via Spring, with the following default validations:

• Must consist of a mixture of lowercase letters and numerical digits only, with at least one of each.
• Must be between 5 and 12 characters in length.
• Must not contain any sequence of characters immediately followed by the same sequence.

Unfortunately they never told me why my implementation is bad, so would someone be kind enough to tell me what I did wrong here?

Here's the main validation class for the library:

package dm.passwordvalidator;

import java.util.ArrayList;
import java.util.List;

/**
 * @author dm
 *
 */
public class PasswordValidator {

    private List<ValidationRule> rules;

    /**
     * Runs all your validation rules and returns true if it's valid, false otherwise
     * @param inPassword
     * @return boolean true if valid, false if otherwise
     */
    public boolean validate(String inPassword) {
        for (ValidationRule rule : this.getRules()) {
            if (!rule.validate(inPassword)) {
                return false;
            }
        }
        return true;
    }

    /**
     * Runs all your validation rules and returns a {@link ValidationResult} with messages if
     * the password was invalid
     * @param inPassword
     * @return {@link ValidationResult} The validation result
     */
    public ValidationResult validateWithMessages(String inPassword) {
        ValidationResult returnValue = new ValidationResult();
        List<String> messages = new ArrayList<String>();
        for (ValidationRule rule : this.getRules()) {
            if (!rule.validate(inPassword)) {
                messages.add(rule.getMessage());
            }
        }
        if (messages.size() > 0) {
            returnValue.setValid(false);
        }
        returnValue.setMessages(messages);
        return returnValue;
    }

    public List<ValidationRule> getRules() {
        return this.rules == null ? this.getDefaultRules() : this.rules;
    }

    public void setRules(List<ValidationRule> inRules) {
        this.rules = inRules;
    }

    private List<ValidationRule> getDefaultRules() {
        List<ValidationRule> returnValue = new ArrayList<ValidationRule>();
        returnValue.add(new RegexValidationRule("(?!(.+?)\\1).*", DefaultValidatorMessages.NO_CYCLES));
        returnValue.add(new RegexValidationRule("[a-z0-9]+", DefaultValidatorMessages.LOWERCASE_AND_NUMBERS));
        returnValue.add(new RegexValidationRule(".*[a-z].*", DefaultValidatorMessages.LOWERCASE_AND_NUMBERS));
        returnValue.add(new RegexValidationRule(".*[0-9].*", DefaultValidatorMessages.LOWERCASE_AND_NUMBERS));
        returnValue.add(new RegexValidationRule(".{5,15}", DefaultValidatorMessages.FIVE_AND_TWELVE));
        return returnValue;
    }

} 

The library is configurable as beans like this:

<bean id="lengthRule" class="dm.passwordvalidator.RegexValidationRule">
    <property name="regex" value=".{5,15}"/>
    <property name="message" value="Your password needs to be between 5 to 15 characters"/>
</bean>

<bean id="passwordValidator" class="dm.passwordvalidator.PasswordValidator">
    <property name="rules">
        <list>
            <ref bean="lengthRule" />             
        </list>
    </property>
</bean>

Answer 1

Putting this as an anwer, because it took a while to find it:

I took your code, and put it through some tests..... I made the tests up based on the 'rules'. This is the sort of thing any interviewer will do:

public static void main(String[] args) {
    String[] positive = {"12345abc", "1a2b3c4d", "1234567890ab", "1234a"};
    String[] negative = {"", "1", "1a", "123567890", "abcdefghij", "abcdEfgh1", "abcabc123", "123a", "1234567890abc"};


    PasswordValidator validator = new PasswordValidator();

    for (String pos : positive) {
        ValidationResult result = validator.validateWithMessages(pos);
        if (!result.isValid()) {
            System.out.println("Claims that '" + pos + "' is not valid!: " + result.getMessages());
        }
    }

    for (String neg : negative) {
        if (validator.validate(neg)) {
            System.out.println("Claims that '" + neg + "' is valid (but it is not)!");
        }
    }
}

When I ran that test, it passed the password: "1234567890abc" which is 13 characters long, and the rules say the limit is 12.

Checking your Regex, I see you mis-typed the range-limit on the match: ".{5,15}"

Hate to be the bearer of bad news, but an interviewer could look at this as being an indication that you are not 'detail-oriented', and dismiss the application immediately.

For what it's worth, I look at your code and feel it has a strong structure, a reusable and extensible model. Generally good. Perhaps overkill, but that's OK.

But, bottom line, if your code does not meet the specification, you can expect to be moved down the pile of applicants ......

P.S. It also blew up on a null password, which I removed from my test because I did not think it was 'fair', but regardless, you should handle null input gracefully.

Answer 2

It looks fine. Some minor improvement idea:

1. The first regexp rule ((?!(.+?)\\1).*) is really hard to understand. I'd create a named local variable for it to explain its intent.

   Some reference: Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler:

   Put the result of the expression, or parts of the expression, in a temporary variable with a name that explains the purpose.

   Furthermore, Clean Code by Robert C. Martin, G19: Use Explanatory Variables.

2. !messages.isEmpty() would be a little bit more readable than messages.size() > 0.

3. Comments like this are unnecessary:

   * @param inPassword
   

4. I'd consider moving the default values to a separate class to fulfill the single responsibility principle. Currently it could be changed because of changes in password validation or changes in the default validation rules.