Gfrktyl

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

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.

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());

            }

        }

    }

}

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
   
   

   
   

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()));

        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);