public static non-final variables are destroying your Zen, dude!
(No offense meant, it's a serious message in a funny wrapping meant to be easy to remember.)
Data hiding
Java is a language which provides so many big and small things which help us with encapsulation and data hiding. We want to encapsulate and hide data because that minimizes the risks of wrong coupling and bogus dependencies. There are 4 big design smells in the world, and almost all problems which software development projects face can be tracked down to these 4 design smells: Fragility, Rigidity, Immobility and Viscosity. More than 3.0 of these 4 design smells are primarily caused by coupling and dependency.
public static non-final variables are globally accessible mutable variables, or short global variables, and we don't want global variables.
Consider making your variables local.
Data modeling / reentrance
Think of the reusability of your function. Imagine, some big Internet company like Google, Amazon, Alibaba, Flipkart or whatsoever likes your password check algorithm and wants to reuse it.
static fields exist exactly once. (Strictly speaking they exist once per Class instance, and different ClassLoader could create separate independent such instances, but if you're not familiar with that see this as esoteric stuff for now.)- non-
static fields exist once per object.
- local variables exist once per function call.
This means that from a runtime reusability perspective, static fields are dangerous. What if two users would check their passwords at the same time? Their data would get messed up.
If you create non-static fields, each password check would need its own PassTest object.
If you have local variables, each password check automatically has its own copy of the relevant data.
So, I hope I've given you two good reasons for making your variables local.
Name: Test vs. Validator vs. Analyzer
You call your class PassTest. Actually, the word Test already is reserved with some meaning that's different from how you use the word Test.
A Test is an automated (ya James Bach beat me to it, as if I care :P) reproducible verification of a program's behavior for specified test cases. A test has a result, and that's either "PASS" or "FAIL".
If you want to tell the user whether the password is good or not, you might actually call it Validator instead, or if it's purely about giving some info about the password, Analyzer might be a good name.
So, I'd rename the class to PasswordAnalyzer.
Security: Sniffing / Spying the password
You might want to go for Console.readPassword() instead of BufferedReader(...System.in...).readLine(). The drawback is that Console.readPassword() bypasses stdin and directly reads from the terminal. The upside is that Console.readPassword() disables the terminal echo for the password characters and returns a char[] which is more secure.
Why not show the password on the screen?
Someone might look over your shoulder and see the password.
Why is char[] more secure than String?
Because after use of the char[], you can clear the char[] and fill it with zero - and you should! You cannot do that with a String, because Strings are immutable.
The time between end of password use and garbage collection might be infinitely long, and that's where someone might spy the password in memory.
Chinese Characters
You might want to know that Chinese characters like '噸' are neither uppercase nor lowercase. A proper Java program would allow Chinese to use it as well and produce meaningful results for them, too.
Closing Resources
You might want to dataIn.close() because that's good habit.
In other programs, forgetting to close() or even to flush() actually can lead to bugs which can be very tough to find.
I made it a habit to always finish what I start, which for streams and stuff means I always close what I open as soon as it's unused, even if it seems unnecessary.
The following code snippet shows how to dataIn.close() using try-with-resources in Java7 onwards.
try (final BufferedReader dataIn = new BufferedReader(new InputStreamReader(System.in)) {
// ...
}
The closing brace implicitly calls dataIn.close(), that's how try-with-resources is defined and works. Of course this is only required if, for some reason, you decide to stick to System.in instead of using Console.readPassword().
if-if vs. if-else if
In case things are mutually exclusive, if-else if should be preferred over if - if, for two reasons.
- Performance - the
else if checks are skipped if the if matched.
- Expressing the intent (more important than performance unless requirements + performance analysis report says otherwise and a profile proves that this part of the code is the bottleneck)
The question now is, are isUpperCase(), isLowerCase() and isDigit() mutually exclusive? AFAIK they are, so go for else if instead of if.
Special characters
Are you sure that you have listed them completely? In the year 2015, do you want to limit this to the small bunch of ASCII non-digit non-control non-letters, when the Unicode offers so much more? Why not say: "everything else is a special character"?
Sanitize your input (NullPointerException on dataIn.readLine().length())
What if the user doesn't enter a password but closes the stream (using Ctrl+D on UNIX resp. Ctrl+Z on cmd.exe)?
In that case, dataIn.readLine() returns null, and password.X will throw new NullPointerException no matter what X is.
Therefore you might want to explicitly check that password != null.
Looping over characters of a String
The relevant part of your loop is:
for (int i = 0; i < password.length(); i++) {
... password.charAt(i) ...
... password.charAt(i) ... // again
... password.charAt(i) ... // and again and again...
}
The return value of password.charAt(i) would always be the same, so you can speed up the code and express the intent better at the same time by introducing a local variable and change the relevant part of the code to be like this:
for (int i = 0; i < password.length(); i++) {
final char currentChar = password.charAt(i);
... currentChar ...
... currentChar ...
... currentChar ...
}
You can even go one step further because you're accessing each and every char of the entire String password, not just a few. Therefore, creating a char[] copy of the String comes with no penalty, in fact it makes the code faster - and again more maintainable:
for (final char currentChar : password.toCharArray()) {
... currentChar ...
... currentChar ...
... currentChar ...
}
However, in order to prevent password sniffing, you might want to clear each character as soon as it is analyzed, and thus go for:
final char[] password = Console.readPassword();
for (int i = 0; i < password.length; i++) {
final char currentChar = password[i];
password[i] = 0;
... currentChar ...
... currentChar ...
... currentChar ...
}
Platform-independent newline
In printf(), you're using \n to create a newline. You might want to use %n instead. \n is always LF (0x0A). But depending on the platform, the program should actually print \n (LF, 0x0A, UNIX), \r\n (CRLF, 0x0D 0x0A, DOS / Windows), \r (CR, 0x0D, old Mac) or even something else (weird IBM mainframes with EBCDIC and stuff).
With %n, printf will generate a newline for you with whatever the underlying platform would like to have.
Characters, char vs int, code points
When Java was invented, Unicode looked like 16 Bit.
Today, the Unicode is bigger than that.
A char in Java does not represent a Unicode character in general. It only represents a Unicode character if that character has a code point <65536. Thus analyzing single char values can be insufficient for rare alphabets.
@AJMansfield provided a solution which is functional and uses code points instead of char. Have a look at it. It also takes security into account.
