15
\$\begingroup\$

I have a text file and I was curious about what character appears how often in the text.

Any review is appreciated.

public class CountLetters {
    public static void main(String[] args) throws Exception {
        TreeMap<Character, Integer> hashMap = new TreeMap<Character, Integer>();
        File file = new File("C:/text.txt");
        Scanner scanner = new Scanner(file,"utf-8");
        while (scanner.hasNext()) {
            char[] chars = scanner.nextLine().toLowerCase().toCharArray();
            for (Character c : chars) {
                if(!Character.isLetter(c)){
                    continue;
                }
                else if (hashMap.containsKey(c)) {
                    hashMap.put(c, hashMap.get(c) + 1);
                } else {
                    hashMap.put(c, 1);
                }
            }
        }
        for (Map.Entry<Character, Integer> entry : hashMap.entrySet()) {
            System.out.println(entry.getKey() + ": " + entry.getValue());
        }
    }
}

The output will be for example:

a: 1202
b: 311
c: 603
d: 510
e: 2125
f: 373
g: 362
h: 718
i: 1313
j: 5
k: 74
l: 678
m: 332
n: 1129
o: 1173
p: 348
q: 40
r: 812
s: 1304
t: 1893
u: 415
v: 195
w: 314
x: 86
y: 209
z: 9
\$\endgroup\$
19
\$\begingroup\$

Resources:

You should start using try-with-resources. This statment does some work for you with resources that implement AutoCloseable. It closes these resources for you, so you don't have to worry about file-locks and remaining database connections:

File file = new File("C:/text.txt");
try(Scanner scanner = new Scanner(file, "utf-8")){
    //your code here ;)
}

You also shouldn't throw Exception in the main method of your program. This can be very confusing to users. Instead you main-method should handle all exceptions "gracefully" by being wrapped into a try-catch-block.

Conditionals:

if(!Character.isLetter(c)){
   continue;
}

This is an early return statement for the purpose of following conditions, meaning you don't have to write else if in your next condition.

Naming

hashMap is not a good name. The map you use is not a Hash-Map, and treeMap would also not explain what the map does, what it contains.

You might want to rename it to characterMap

all else equal, your naming is nice and consistent, and tells exactly what the variables do. You nicely follow camelCase-conventions. Keep it up!

Summary:

Your code reads nicely and is easily understandable. You follow naming conventions and have descriptive and understandable variable names. You should work on your exception handling and the use of resources.

\$\endgroup\$
  • \$\begingroup\$ I would name it characters instead of characterMap since that "map" doesn't add anything and instead removes some abstraction from your variable. It will cause a refactoring if you change the underlying datastructure. \$\endgroup\$ – Jeroen Vannevel Apr 20 '14 at 17:25
  • 1
    \$\begingroup\$ @Jeroen ypu have a point. But changing the underlying datastructure will cause a refactoring either way. \$\endgroup\$ – Vogel612 Apr 20 '14 at 17:30
  • \$\begingroup\$ I would use StandardCharsets.UTF_8 rather than the String. Otherwise +1 \$\endgroup\$ – Boris the Spider Apr 20 '14 at 18:41
  • \$\begingroup\$ @BoristheSpider In this case it would require StandardCharsets.UTF_8.toString() not just StandardCharsets.UTF_8. \$\endgroup\$ – user40964 Apr 21 '14 at 9:24
8
\$\begingroup\$

My notes in code:

public class CountLetters {
    // Throwing Exception is too general, you should throw IOException
    public static void main(String[] args) throws Exception {
        // It is better practice to define Map, instead of TreeMap
        // The name of variable hashMap could be better, for example characterMap or characters
        TreeMap<Character, Integer> hashMap = new TreeMap<Character, Integer>();
        File file = new File("C:/text.txt");
        Scanner scanner = new Scanner(file,"utf-8");

        while (scanner.hasNext()) {
            char[] chars = scanner.nextLine().toLowerCase().toCharArray();
            for (Character c : chars) {
                if(!Character.isLetter(c)){
                    // 'continue' is unnecessary as last statement in a loop
                    // It is better to put following 'else if' and 'else' here and to remove negation in condition
                    // like this: if(Character.isLetter(c)){ if ( ... ) { ... } else { ... } }
                    continue;
                }
                else if (hashMap.containsKey(c)) {
                    hashMap.put(c, hashMap.get(c) + 1);
                } else {
                    hashMap.put(c, 1);
                }
            }
        }

        // You should call scanner.close() here

        // I would wrap this into if (!hashMap.isEmpty()) { ... }, but it is not really needed
        for (Map.Entry<Character, Integer> entry : hashMap.entrySet()) {
            System.out.println(entry.getKey() + ": " + entry.getValue());
        }
    }
}

I would rewrite the class like this:

public class CountLetters {
    public static void main(String[] args) throws IOException {
        Map<Character, Integer> characters = new TreeMap<Character, Integer>();
        Scanner scanner = null;

        try {
            scanner = new Scanner(new File("C:/text.txt"),"utf-8");

            while (scanner.hasNext()) {
                char[] line = scanner.nextLine().toLowerCase().toCharArray();

                for (Character character : line) {
                    if (Character.isLetter(character)){
                        if (characters.containsKey(character)) {
                            characters.put(character, characters.get(character) + 1);
                        } else {
                            characters.put(character, 1);
                        }
                    }
                }
            }
        } finally {
            if (scanner != null){
                scanner.close();
            }
        }

        if (!characters.isEmpty()){
            for (Map.Entry<Character, Integer> entry : characters.entrySet()) {
                System.out.println(entry.getKey() + ": " + entry.getValue());
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ It's great to see such a new user being so active! +1, and feel free to join us in our chat room sometime! :) \$\endgroup\$ – syb0rg Apr 20 '14 at 16:07
8
\$\begingroup\$
  1. I would change the map hashMap to characterCounterMap.
  2. Then I would initialized the map at first with characters, like

    for(char c = 'a'; c <= 'z'; c++) {
        characterCounterMap.put(c,0);
    }
    
  3. Then it will help you to shortening the if-else ladder, like

    if(Character.isLetter(character)) {
        characterCounterMap.put(character, characterCounterMap.get(character) + 1);
    } // see no else
    
\$\endgroup\$
  • 4
    \$\begingroup\$ Caution! Character.isLetter() is Unicode-aware, and supports more than just 'a'..'z'. \$\endgroup\$ – 200_success Apr 21 '14 at 8:37
8
\$\begingroup\$

All the points in Vogel612's answer should be taken into consideration. Your failure to close resources is your biggest issue.

My main aim with this answer is to show how how this should now be done with Java 8.

Your current method uses very traditional Java loops and conditions. Here is how the code should look with the Java 8 APIs:

public static void main(final String[] args) {
    final Path file = Paths.get("C:/text.txt");
    try (final Stream<String> lines = Files.lines(file)) {
        final Map<Character, Integer> count = lines.
                flatMap(line -> IntStream.range(0, line.length()).mapToObj(line::charAt)).
                filter(Character::isLetter).
                map(Character::toLowerCase).
                collect(TreeMap::new, (m, c) -> m.merge(c, 1, Integer::sum), Map::putAll);
        count.forEach((letter, c) -> System.out.println(letter + ": " + c));
    } catch (IOException e) {
        System.out.println("Failed to read file.");
        e.printStackTrace(System.out);
    }
}

This code has exactly the same function as your code but is significantly shorter - it leverages Java 8's new Stream API combined with the all new lambdas.

The code uses Files.lines to get a Stream<String> consisting of each line. It then uses flatMap to turn that into a Stream<Character> by "flattening" a Stream<Stream<Character>> which we get by creating an IntStream of [0, line.length()) and calling line.charAt for each element of the IntStream. The char is then autoboxed to a Character.

We use the filter method of Stream to strip out things that aren't letters.

Now we use the new Map.merge method with takes a key and a value and in addition a lambda that takes two values. If the key does not exist in the map then it is simply added with the given value. If it does exist in the map then the lambda is called with the existing value and the new value; the value returned from the lambda is then put into the map.

We use the collect method of the Stream<Character> to "reduce" the stream to a mutable collection, in this case a TreeMap.

Finally we use the new forEach method on Map to print out the contents of the map.

As a demonstration of the power of Java 8, in order to sort the output by count rather than by the character (as you had in your post), simply change the print to:

count.entrySet().stream().
        sorted((l, r) -> l.getValue().compareTo(r.getValue())).
        forEach(e -> System.out.println(e.getKey() + ": " + e.getValue()));
\$\endgroup\$
  • \$\begingroup\$ This is really a nice rewrite to Java 8. \$\endgroup\$ – user40964 Apr 21 '14 at 9:27
  • \$\begingroup\$ Something I just noticed... what's the reason you're flatmapping over an IntStream that calls into charAt instead of using Arrays.stream onto toCharArray? flatMap(line -> Arrays.stream(line.toCharArray()) should work just as well and is IMO cleaner... \$\endgroup\$ – Vogel612 Sep 20 '16 at 8:42
  • 1
    \$\begingroup\$ @Vogel612 you may have a point - I remember having lots of issues with char and int and boxing/unboxing; the only downside I can see from using toCharArray is that it creates a new char[] - but creating the new InterStream might well be even worse. Another issue I spotted, now that I'm looking at it, is that in the last part (l, r) -> l.getValue().compareTo(r.getValue()) could simply be Map.Entry.comparingByValue(). \$\endgroup\$ – Boris the Spider Sep 20 '16 at 9:11
  • \$\begingroup\$ but is significantly shorter I see that you consider this to be an advantage, but I do not believe shorter code is always better . To me it is usually more difficult to read, to runtime it does not matter at all anyway.. So where is the advantage? \$\endgroup\$ – Koray Tugay Feb 6 '18 at 19:43
  • \$\begingroup\$ @KorayTugay I believe it is only more difficult to use because you are not used to reading it. I think the above Java 8 code is more terse and expresses semantic meaning much better - you read what the intent of the code is rather than all the boilerplate around the code. \$\endgroup\$ – Boris the Spider Feb 6 '18 at 21:12
0
\$\begingroup\$
        TreeMap<Character, Integer> hashMap = new TreeMap<Character, Integer>();

In Java, we prefer types to be interfaces rather than implementations. In this case, the interface could be Map, but

        SortedMap<Character, Integer> characterCounts = new TreeMap<>();

I think that you actually want SortedMap, as otherwise there is no need to use a TreeMap. You could even use NavigableMap, although you don't seem to be using that functionality.

In modern Java versions, you don't need to specify the types in <> twice. The compiler will match the second to the first automatically.

I changed the name to characterCounts, as that better describes the data that it holds.

                if(!Character.isLetter(c)){
                    continue;
                }
                else if (hashMap.containsKey(c)) {
                    hashMap.put(c, hashMap.get(c) + 1);
                } else {
                    hashMap.put(c, 1);
                }

Since you continue in the first clause, you don't need an else. You can instead say something like

                if (!Character.isLetter(c)) {
                    continue;
                }

                Integer count = characterCounts.get(c);
                if (count == null) {
                    count = 0;
                }

                count++;
                characterCounts.put(c, count);

Now we don't have two put statements. And we don't call contains explicitly just to call get, which calls contains implicitly. We simply treat a null count as if it were zero. The rest of our logic is the same in both paths.

I find the explicit increment easier to read than adding one.

I added some whitespace for readability.

\$\endgroup\$

protected by Jamal Jan 2 at 8:30

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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