3
\$\begingroup\$

I'm making a Java program that will analyze the given String by counting the lowercase, uppercase, numbers and special characters. Is there a way by making it functional?

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.io.IOException;


public class PassTest {

    public static String password = "";
    public static int upperCase = 0;
    public static int lowerCase = 0;
    public static int numberCount = 0;
    public static final String FINAL_CHAR_REGEX = "[!@#$%^&*()[\\\\]|;',./{}\\\\\\\\:\\\"<>?]";

public static void main(String[] args) throws IOException {
    BufferedReader dataIn = new BufferedReader(new InputStreamReader(System.in));
        System.out.print("Enter your password: ");
             password = dataIn.readLine();
            for ( int i = 0; i < password.length() ; i++ ) {
                if (Character.isUpperCase(password.charAt(i))){ upperCase++; }
                if (Character.isLowerCase(password.charAt(i))){ lowerCase++; }
                if (Character.isDigit(password.charAt(i)))    { numberCount++;}
            }
        int specialCharCount = password.split(FINAL_CHAR_REGEX, -1).length - 1;
            System.out.printf("Your password contains %d uppercases, %d lowercases, %d digits and %d special characters.\n\n", upperCase, lowerCase, numberCount, specialCharCount);
        ;
       }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ What do you mean by functional? \$\endgroup\$ – David Jan 10 '15 at 12:37
5
\$\begingroup\$

First,

        System.out.printf("Your password contains %d uppercases, %d lowercases, %d digits and %d special characters.\n\n", upperCase, lowerCase, numberCount, specialCharCount);
    ;
   }

I don't think there is any need for that empty ";". I will think that you didn't notice that it's there.

        for ( int i = 0; i < password.length() ; i++ ) {

Since in every iteration you're testing for each char, why not make a foreach with chars?

        for (char c : password.toCharArray()) {
        //Now the loop and if statements can be shorter and simpler:
            if (Character.isUpperCase(c)){ upperCase++; }

This can be improved:

            if (Character.isUpperCase(password.charAt(i))){ upperCase++; }
            if (Character.isLowerCase(password.charAt(i))){ lowerCase++; }
            if (Character.isDigit(password.charAt(i)))    { numberCount++;}

All of these expressions will execute each iteration. This need not be the case because there is no character that can have two characteristics (E.G. if a char is lowercase, it can't be uppercase or a digit). Put them in if else condition, like this.

            if (Character.isUpperCase(password.charAt(i))){ upperCase++; }
            //if the above condition is true (if char is uppercase, stops and loop iterates). Else, execute below code until one results true
            else if (Character.isLowerCase(password.charAt(i))){ lowerCase++; }
            else if (Character.isDigit(password.charAt(i)))    { numberCount++;}

This is Unnecessary:

public static String password = "";

You're only using the value once (count it's characters), so why store it?

Looks like this has a bug:

public static final String FINAL_CHAR_REGEX = "[!@#$%^*()[\\\\]|;',./{}\\\\\\\\:\\\"<>?]";

I know nothing about regex. But it's not detecting all special characters. For example,

Enter your password: a.
Your password contains 0 uppercases, 1 lowercases, 0 digits and 1 special characters.
Enter your password: a&&&&&&&&&
Your password contains 0 uppercases, 1 lowercases, 0 digits and 0 special characters. //totally wrong.

But why would you use regex? I might be wrong, if a char is not a letter and not a digit, Isn't it a special character? :)

I would test for special characters inside your loop.

Here's how I would right your method.

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.io.IOException;

public class PassTest {

    public static int upperCase = 0;
    public static int lowerCase = 0;
    public static int numberCount = 0;
    public static int specialCharCount = 0;

    public static void main(String[] args) throws IOException {
        BufferedReader dataIn = new BufferedReader(new InputStreamReader(System.in));
        System.out.print("Enter your password: ");
        for (char c : dataIn.readLine().toCharArray()) {
            if (Character.isUpperCase(c)){ upperCase++; }
            else if (Character.isLowerCase(c)){ lowerCase++; }
            else if (Character.isDigit(c)){ numberCount++;}
            else { specialCharCount++; }
        }
        System.out.printf("Your password contains %d uppercases, %d lowercases, %d digits and %d special characters.\n\n", upperCase, lowerCase, numberCount,
                specialCharCount);
    }
}
\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

Not sure I understand what you mean by 'more functional', but my attempt would look something like this, separating classification from actual processing:

public class PassTest {
    enum CharType {UPPER, LOWER, DIGIT, SPECIAL};

    static CharType charType(int codePoint){
        if (Character.isUpperCase(codePoint)) return CharType.UPPER;
        else if (Character.isLowerCase(codePoint)) return CharType.LOWER;
        else if (Character.isDigit(codePoint)) return CharType.DIGIT;
        else return CharType.SPECIAL;
    }

    public static void main(String[] args) throws IOException {
        BufferedReader dataIn = new BufferedReader(new InputStreamReader(System.in));
        System.out.print("Enter your password: ");

        Map<CharType, Integer> counts = dataIn.readLine().codePoints()
                .collect(Collectors.groupingBy(PassTest::charType, Collectors.counting()));

        System.out.printf("Your password contains %d uppercases, %d lowercases, %d digits and %d special characters.\n\n",
                counts.get(CharType.UPPER), counts.get(CharType.LOWER), counts.get(CharType.DIGIT), counts.get(CharType.SPECIAL));
    }
}

In response to comments below:

This shares a security issue with the OP's implementation, in that a BufferedReader nessisarily stores the characters in a buffer, allowing memory sniffing attacks to read it if it is not overwritten. Further, the readLine method returns its output as a String, which creates another copy of the line elsewhere, also out of your control.

To do this more securely and preventing memory sniffing attacks, you can use a Console to read the password as a char[] (and also ensure the characters are not printed to the screen as they are typed). You can then use a Segment to provide the codePoints() method. Segments are basically unprotected strings, and can be constructed to use an existing char[] without allocating a new one. After use, you can then zero the array, reducing the window for memory-sniffing attacks.

public static void main(String[] args) throws IOException {
    Console c = System.console();

    char [] password = c.readPassword("Enter your password: ");

    Map<CharType, Integer> counts = new Segment(password,0,password.length).codePoints()
            .collect(Collectors.groupingBy(PassTest::charType, Collectors.counting()));

    Arrays.fill(password, 0);

    c.printf("Your password contains %d uppercases, %d lowercases, %d digits and %d special characters.\n\n",
            counts.get(CharType.UPPER), counts.get(CharType.LOWER), counts.get(CharType.DIGIT), counts.get(CharType.SPECIAL));
}
\$\endgroup\$
  • \$\begingroup\$ Good solution for 'more functional', got my upvote. Do you happen to know how well this IntStream codePoints() performs from a security perspective? I'm eyeing on the same reason for which Console.readPassword() is returning char[] instead of String - that you can reliably zero-erase the memory that contains the password to prevent memory sniffing attacks. \$\endgroup\$ – Christian Hujer Jan 10 '15 at 18:36
  • \$\begingroup\$ @ChristianHujer I think it is probably secure. Most streams, including the one returned by CharSequence.codePoints are lazily evaluated (like most iterators), so the sequence of integers is never copied to another location. Since it is a IntStream and not just a generic Stream<Integer>, the code points stay as primitives (no Integer boxes get allocated). In order to do this securely with a char[] though, you would need to use a CharSequence implementation that does not copy the array, such as a Segment. \$\endgroup\$ – AJMansfield Jan 10 '15 at 18:53
  • \$\begingroup\$ Cool. Do you also happen to know how I could get an IntStream with the code points of a char[] without copying them? I looked in IntStream (unlikely), Character, CharSequence and the usage page of IntStream but couldn't find anything. Looks like I'd have to deal with my own IntStream.Builder or IntStream based on calls to Character, isn't it? Feels "incomplete". \$\endgroup\$ – Christian Hujer Jan 10 '15 at 18:58
  • 1
    \$\begingroup\$ @ChristianHujer Edited answer to include a more secure implementation. \$\endgroup\$ – AJMansfield Jan 10 '15 at 19:15
  • \$\begingroup\$ Using javax.swing.text.Segment for that looks like abuse. Not your fault. This class Segment has no dependency on Swing and is clearly misplaced / in the wrong package. It should be in java.lang, java.text or java.util. I've searched around more and found that CharBuffer.wrap() should also do the job, what do you think? Somehow, using something from java.nio sounds more reasonable to me for this problem than using something from javax.swing. \$\endgroup\$ – Christian Hujer Jan 10 '15 at 21:50

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.