7
\$\begingroup\$

There is a user login, and the requirements are the following:

  • The login must start with a Latin letter.
  • The login must finish with either a Latin letter or a digit.
  • There may also be digits, dots, and minus signs in the login.
  • Min. login length is 1.
  • Max. login length is 20.

Here is my Java code.

boolean checkLogin(String login) {
        final int maxLength = 20;
        if (login == null || login.isEmpty() || login.length() > maxLength) {
            return false;
        }

        final Pattern pattern = Pattern.compile("^[a-z][a-z\\d\\.\\-]*[a-z\\d]+$", Pattern.CASE_INSENSITIVE);
        final Matcher matcher = pattern.matcher(login);
        return matcher.matches();
}

Is everything OK?

\$\endgroup\$
4
\$\begingroup\$

That regex does not meet the requirements of your rules... so ... No, it is not OK.

The missing aspect is that the regex requires at least two characters, but the rules say one char is OK.

Also, I don't like that the last group has the + on it. While it is accurate in the sense that the last characters can be alpha/digit, it implies that the rule is for more than just the last character.

Another problem is that you allow logins of more than 20 characters.

A more meaningful/accurate regex would be:

^[a-z]([a-z\\d\\.\\-]{0,18}[a-z\\d])?$

Which requires 1 character, and then up to 18 'broad' characters followed by a constrained alpha/digit character.

EDIT: 2 things:

  1. Aseem has rightly pointed out that the preceding if-condition essentially makes the {0,18} limit redundant. My preference is to keep it there for clarity, but, it does create duplication of logic which may lead to bugs later (How did I miss that the lenght is checked first... hmmm). Your call as to whether you should replace the {0,18} with just {*}.
  2. The Pattern should be made static. There is no reason to re-compile the pattern for every call to the method (Patterns are thread-safe):

    private static final Pattern pattern = Pattern.compile(
            "^[a-z]([a-z\\d\\.\\-]{0,18}[a-z\\d])?$", Pattern.CASE_INSENSITIVE);
    
    private static final int maxLength = 20;
    
    boolean checkLogin(String login) {
    
        if (login == null || login.isEmpty() || login.length() > maxLength) {
            return false;
        }
    
        return pattern.matcher(login).matches();
    }
    
\$\endgroup\$
  • \$\begingroup\$ How does it allow logins of more than 20 characters? Isn't that checked in the if? Your approach is better but.. \$\endgroup\$ – Aseem Bansal Dec 3 '13 at 18:04
  • \$\begingroup\$ @AseemBansal - yes, the preceding if condition makes the {0,18} redundant... hmmm. The {0,18} can be replaced with * but there is a very slight performance benefit for the exactly-20 char login if you constrain to 18.... so slight it's not worth considering.... \$\endgroup\$ – rolfl Dec 3 '13 at 18:07
  • \$\begingroup\$ @rolfl, Why do use a group in your reply? It could look like this ^[a-z][a-z\d\.\-]{0,18}[a-z\d]$ I guess that the group makes it work faster \$\endgroup\$ – Maksim Dmitriev Dec 3 '13 at 18:09
  • 1
    \$\begingroup\$ @MaksimDmitriev That makes it minimum 2 characters \$\endgroup\$ – Aseem Bansal Dec 3 '13 at 18:09
  • 1
    \$\begingroup\$ @AseemBansal I have edited to add a couple more suggestions. \$\endgroup\$ – rolfl Dec 3 '13 at 18:14
4
\$\begingroup\$

A good naming convention for predicates (functions that return a boolean and that have no side-effects) is isBlah(). I would rename the method to isValidLogin().

If you would like to use a regular expression, then let the regular expression do the hard work. You can even limit the length using the regex. The only thing the regular expression matcher can't handle is a null string.

Compile the pattern just once as a class constant.

Your regex is wrong. Within a character class (i.e., between […]), the language is different: . is a literal dot. The hyphen is also literal if it comes at the end of the character class. Therefore, no backslashes are necessary. Also, your regex requires at least two characters, though your rules require just one.

Here is how I would write it:

private static final LOGIN_PATTERN =
    Pattern.compile("^[a-z](?:[a-z0-9.-]{0,18}[a-z0-9])?$", Pattern.CASE_INSENSITIVE);

boolean isValidLogin(String login) {
    return (login != null) && LOGIN_PATTERN.matcher(login).matches();
}
\$\endgroup\$
0
\$\begingroup\$

The regexes

"^[a-z][a-z\\d\\.\\-]*[a-z\\d]+$"

"^[a-z][a-z0-9.-]*[a-z0-9]$"

are basically the same. True story.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.