Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question

4 Answers 4

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.

share|improve this answer
    
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. –  Jeroen Vannevel Apr 20 at 17:25
    
@Jeroen ypu have a point. But changing the underlying datastructure will cause a refactoring either way. –  Vogel612 Apr 20 at 17:30
    
I would use StandardCharsets.UTF_8 rather than the String. Otherwise +1 –  Boris the Spider Apr 20 at 18:41
    
@BoristheSpider In this case it would require StandardCharsets.UTF_8.toString() not just StandardCharsets.UTF_8. –  Yoda Apr 21 at 9:24
  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
    
share|improve this answer
1  
Caution! Character.isLetter() is Unicode-aware, and supports more than just 'a'..'z'. –  200_success Apr 21 at 8:37

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());
            }
        }
    }
}
share|improve this answer
    
It's great to see such a new user being so active! +1, and feel free to join us in our chat room sometime! :) –  syb0rg Apr 20 at 16:07

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()));
share|improve this answer
    
This is really a nice rewrite to Java 8. –  Yoda Apr 21 at 9:27

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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