4
\$\begingroup\$

I am trying to read a sequence of random characters, integers and symbols from the standard inputstream, split it into 3 separate arrays (uppercase, lowercase and numeric) and print them in that order using standard Java API and JDK7.

Is there any other(better) way of dong the same?

BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(System.in));
String characters = bufferedReader.readLine();

char[] chars = characters.toCharArray();
List<Character> characterList = new ArrayList<>(chars.length);

for (char c : chars) {
    if (Character.isUpperCase(c)) {
        characterList.add(c);
    } else if (Character.isLowerCase(c)) {
        characterList.add(c);
    } else if (Character.isDigit(c)) {
        characterList.add(c);
    }
}

StringBuilder uppercaseStringBuilder = new StringBuilder();
StringBuilder lowercaseStringBuilder = new StringBuilder();
StringBuilder numericStringBuilder = new StringBuilder();

for (Character character : characterList) {
    if (Character.isUpperCase(character)) {
        uppercaseStringBuilder.append(character);
    } else if (Character.isLowerCase(character)) {
        lowercaseStringBuilder.append(character);
    } else if (Character.isDigit(character)) {
        numericStringBuilder.append(character);
    }
}

char[] uppercaseChars = uppercaseStringBuilder.toString().toCharArray();
char[] lowercaseChars = lowercaseStringBuilder.toString().toCharArray();
char[] numericChars = numericStringBuilder.toString().toCharArray();


for (char uc : uppercaseChars) {
    System.out.print(uc);
}

System.out.println();

for (char lc : lowercaseChars) {
    System.out.print(lc);
}

System.out.println();

for (char nos : numericChars) {
    System.out.print(nos);
}
}
\$\endgroup\$
6
\$\begingroup\$

Points on your code

  1. You are specifically asking for help regarding JDK 1.7. I think that you should learn about a better/simpler approach to taking input: using the Scanner class. It was added in JDK 1.5, which can be as an alternative to the BufferedReader approach. Prior to it (i.e. in the times of JDK 1.4), your approach was the one universally accepted. You did not include your full code here. In order to make your program compile, you must add a throws IOException or surround your code with a try-catch block. You don't need all that using Scanner.
  2. To a Java developer, characters is the name of a character array (char[]), ints or integers is int[], doubles is of type double[], and so on. Hence, it is generally a bad practice to name a String as characters. Use inputString or string or dataString or something like that.
  3. In one place you are using overly long variable names like uppercaseStringBuilder and in another, there are names like uc. The right path is somewhere in between. upperCaseBuilder and upperCaseChar seem readable. Mind that i, j (and sometimes k) are accepted integer variable names (usually used as for loop counters). Similarly, for other primitives as well, c is accepted as a char, d as a double. But it is recommended to use more descriptive names.
  4. There is a principle called DRY, i.e. Don't Repeat Yourself. You can simply add the characters to the necessary Builders when you process them for the first time.
  5. You can entirely bypass the use of List. It is redundant in this case.
  6. The || operator can reduce your if-else ladder in the first loop to a single if statement as: if (Character.isUpperCase(c) || Character.isLowerCase(c)) || Character.isDigit(c).

The Refactored Code

The Java 7 version:

import java.util.Arrays;
import java.util.Scanner;

public class CharacterClassifier {
    public static void main(String... args) {
        // See... no need for "throws IOException" in the signature.
        Scanner scanner = new Scanner(System.in);
        // add some prompt for the user... I didn't
        String data = scanner.nextLine();
        // As simple as that.

        StringBuilder lowerCaseBuilder = new StringBuilder(data.length());
        StringBuilder upperCaseBuilder = new StringBuilder(data.length());
        StringBuilder numericBuilder = new StringBuilder(data.length());

        for (char character : data.toCharArray()) {
            if (Character.isLowerCase(character)) {
                lowerCaseBuilder.append(character);
            } else if (Character.isUpperCase(character)) {
                upperCaseBuilder.append(character);
            } else if (Character.isDigit(character)) {
                numericBuilder.append(character);
            }
        }
        System.out.println("The upper case characters are : ");
        char[] upperCaseChars = upperCaseBuilder.toString().toCharArray();
        System.out.println(Arrays.toString(upperCaseChars));
        // the preferred approach: "toString" is called implicitly
        // System.out.println(upperCaseBuilder);

        System.out.println("The lower case characters are : ");
        char[] lowerCaseChars = lowerCaseBuilder.toString().toCharArray();
        System.out.println(Arrays.toString(lowerCaseChars));
        // System.out.println(lowerCaseBuilder);

        System.out.println("The numeric characters are : ");
        char[] numericChars = numericBuilder.toString().toCharArray();
        System.out.println(Arrays.toString(numericChars));
        // System.out.println(numericBuilder);

    }
}

A Few More Points

I would recommend that you go through the docs to gain an idea of the available options that you we have. But the code can be made ever shorter using the streams and lambdas in Java 8. I suggest that you learn about them. Here is the code:

import java.util.Scanner;

public class CharacterClassifier {
    public static void main(String... args) {
        Scanner scanner = new Scanner(System.in);
        String data = scanner.nextLine();

        System.out.print("\nThe upper case characters are : ");
        data.chars()
            .filter(Character::isUpperCase)
            .forEach(x -> System.out.print((char) x));
        System.out.print("\nThe lower case characters are : ");
        data.chars()
            .filter(Character::isLowerCase)
            .forEach(x -> System.out.print((char) x));
        System.out.print("\nThe numeric characters are : ");
        data.chars()
            .filter(Character::isDigit)
            .forEach(x -> System.out.print((char) x));

        System.out.println();
    }
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I like that you are initializing the StringBuilder with a capacity, many people forget about that optimization. \$\endgroup\$ – Simon Forsberg Sep 12 '15 at 17:47
8
\$\begingroup\$

First obvious thing: Why do you do the almost same thing twice?

for (char c : chars) {
    if (Character.isUpperCase(c)) {
        characterList.add(c);
    } else if (Character.isLowerCase(c)) {
        characterList.add(c);
    } else if (Character.isDigit(c)) {
        characterList.add(c);
    }
}

...and...

for (Character character : characterList) {
    if (Character.isUpperCase(character)) {
        uppercaseStringBuilder.append(character);
    } else if (Character.isLowerCase(character)) {
        lowercaseStringBuilder.append(character);
    } else if (Character.isDigit(character)) {
        numericStringBuilder.append(character);
    }
}

You are going through an array, check if you want the character and add it to a list. Then you go through that list and make the same check again to put the character in different lists / StringBuilders. You can simply remove the last of these loops and add the characters directly from the array to the stringbuilders.

Also your output would be pretty much the same if you simple wrote, for example...

System.out.println(uppercaseStringBuilder.toString());

I guess, with Java8, your problem could be solved nicely (or not so nicely, since char primitives are a little bit tricky there) via Streams.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ actually you could run three separate streams filtered through method references to Character::isDigit, Character::isUpperCase and Character::isLowerCase... but then you'd iterate over the Sequence thrice, instead of once (assuming the for loop is eliminated as you correctly say) \$\endgroup\$ – Vogel612 Sep 12 '15 at 8:34
  • \$\begingroup\$ .toString() is redundant as it is called implicitly. You can simply use: System.out.println(uppercaseStringBuilder); \$\endgroup\$ – Hungry Blue Dev Sep 13 '15 at 7:35
  • \$\begingroup\$ Of course it is, but I prefer it that way to make clear that I actually want that functionality instead of just relying on it as a side effect. Just a personal thing, I guess. \$\endgroup\$ – Florian Schaetz Sep 13 '15 at 7:49

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.