Skip to main content
added 271 characters in body
Source Link
maaartinus
  • 13.7k
  • 1
  • 36
  • 75

In general, not bad.

logger.info(pass1 + " = " + pass2);

Logging passwords may be a security issue.

boolean hasUppercase = !pass1.equals(pass1.toLowerCase());

If you wanted to know if there's a black swan, would you paint all swans white and look if something changed?

boolean hasNumber = pass1.matches(".*\\d.*");
boolean noSpecialChar = pass1.matches("[a-zA-Z0-9 ]*");

Each of those lines would be fine, but together... 3 different ways for doing about the same thing. There are regexes for everything (ok, not really):

boolean hasUppercase = pass1.matches(".*[A-Z].*");

(assuming ASCII, otherwise there's a unicode group, too)

boolean hasSpecialChar = pass1.matches(".*[^0-9A-Za-z].*");

I negated you condition (so it's like the others) and the ^ negates the chargroup.

 logger.info(pass1 + " is length < 11");

A proper logger allows to write

 logger.info("{} is length < 11", pass1);

It's a bit more readable and much faster in case logging is off (and most logging is off most of the time).

The code is pretty repetitive. Can't you the returned value instead of the very similar message?

logger.info("Passwords = null");

Most of the time the best null handling is

Preconditions.checkNotNull(pass1);

which throws an NPEwhich throws an NPE if passed null. Most methods should NOT allow null and throwing ASAP is the best - you fix the problem immediately and don't have to hunt it down later. Use

Strings.nullToEmpty(s);

to get rid of nullget rid of null strings where you can't throw.

In general, not bad.

logger.info(pass1 + " = " + pass2);

Logging passwords may be a security issue.

boolean hasUppercase = !pass1.equals(pass1.toLowerCase());

If you wanted to know if there's a black swan, would you paint all swans white and look if something changed?

boolean hasNumber = pass1.matches(".*\\d.*");
boolean noSpecialChar = pass1.matches("[a-zA-Z0-9 ]*");

Each of those lines would be fine, but together... 3 different ways for doing about the same thing. There are regexes for everything (ok, not really):

boolean hasUppercase = pass1.matches(".*[A-Z].*");

(assuming ASCII, otherwise there's a unicode group, too)

boolean hasSpecialChar = pass1.matches(".*[^0-9A-Za-z].*");

I negated you condition (so it's like the others) and the ^ negates the chargroup.

 logger.info(pass1 + " is length < 11");

A proper logger allows to write

 logger.info("{} is length < 11", pass1);

It's a bit more readable and much faster in case logging is off (and most logging is off most of the time).

The code is pretty repetitive. Can't you the returned value instead of the very similar message?

logger.info("Passwords = null");

Most of the time the best null handling is

Preconditions.checkNotNull(pass1);

which throws an NPE. Most methods should NOT allow null and throwing ASAP is the best - you fix the problem immediately and don't have to hunt it down later. Use

Strings.nullToEmpty(s);

to get rid of null strings where you can't throw.

In general, not bad.

logger.info(pass1 + " = " + pass2);

Logging passwords may be a security issue.

boolean hasUppercase = !pass1.equals(pass1.toLowerCase());

If you wanted to know if there's a black swan, would you paint all swans white and look if something changed?

boolean hasNumber = pass1.matches(".*\\d.*");
boolean noSpecialChar = pass1.matches("[a-zA-Z0-9 ]*");

Each of those lines would be fine, but together... 3 different ways for doing about the same thing. There are regexes for everything (ok, not really):

boolean hasUppercase = pass1.matches(".*[A-Z].*");

(assuming ASCII, otherwise there's a unicode group, too)

boolean hasSpecialChar = pass1.matches(".*[^0-9A-Za-z].*");

I negated you condition (so it's like the others) and the ^ negates the chargroup.

 logger.info(pass1 + " is length < 11");

A proper logger allows to write

 logger.info("{} is length < 11", pass1);

It's a bit more readable and much faster in case logging is off (and most logging is off most of the time).

The code is pretty repetitive. Can't you the returned value instead of the very similar message?

logger.info("Passwords = null");

Most of the time the best null handling is

Preconditions.checkNotNull(pass1);

which throws an NPE if passed null. Most methods should NOT allow null and throwing ASAP is the best - you fix the problem immediately and don't have to hunt it down later. Use

Strings.nullToEmpty(s);

to get rid of null strings where you can't throw.

Source Link
maaartinus
  • 13.7k
  • 1
  • 36
  • 75

In general, not bad.

logger.info(pass1 + " = " + pass2);

Logging passwords may be a security issue.

boolean hasUppercase = !pass1.equals(pass1.toLowerCase());

If you wanted to know if there's a black swan, would you paint all swans white and look if something changed?

boolean hasNumber = pass1.matches(".*\\d.*");
boolean noSpecialChar = pass1.matches("[a-zA-Z0-9 ]*");

Each of those lines would be fine, but together... 3 different ways for doing about the same thing. There are regexes for everything (ok, not really):

boolean hasUppercase = pass1.matches(".*[A-Z].*");

(assuming ASCII, otherwise there's a unicode group, too)

boolean hasSpecialChar = pass1.matches(".*[^0-9A-Za-z].*");

I negated you condition (so it's like the others) and the ^ negates the chargroup.

 logger.info(pass1 + " is length < 11");

A proper logger allows to write

 logger.info("{} is length < 11", pass1);

It's a bit more readable and much faster in case logging is off (and most logging is off most of the time).

The code is pretty repetitive. Can't you the returned value instead of the very similar message?

logger.info("Passwords = null");

Most of the time the best null handling is

Preconditions.checkNotNull(pass1);

which throws an NPE. Most methods should NOT allow null and throwing ASAP is the best - you fix the problem immediately and don't have to hunt it down later. Use

Strings.nullToEmpty(s);

to get rid of null strings where you can't throw.