3
\$\begingroup\$

This program counts the number of the occurence of each character that is used in a file.

I tried to use everything that Java offers to me properly. If you have tips which library functions would have been better, how to make the code more readable, than let me know. If you have a tip how to improve the performance I would be very thankful, also if the readability would decrease than.

import java.util.Map;
import java.util.HashMap;
import java.io.File;
import java.io.FileReader;
import java.io.BufferedReader;
import java.io.IOException;

public class Main {
    public static void main(String[] args) {
        if (args[0].contains("help")) {
            System.out.println("usage: java Main filename.txt");
        }

        Map<Character, Integer> map = new HashMap<>();
        char[] chars = fileToCharList(args[0]);
        for (int i = 0; i < chars.length; i++) {
            if (map.containsKey(chars[i])) {
                Integer number = map.get(chars[i]);
                map.put(chars[i], ++number);
            } else {
                map.put(chars[i], 1);
            }
        }

        for (Map.Entry<Character, Integer> entry : map.entrySet()) {
            System.out.println(entry.getKey() + ": " + entry.
              getValue());
        }
    }

    private static char[] fileToCharList(String fileName) {
        File file = new File(fileName);
        StringBuilder sb = new StringBuilder();
        try (BufferedReader br = new BufferedReader(new FileReader(
          file))) {
            String line;
            while ((line = br.readLine()) != null) {
                sb.append(line);
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
        return sb.toString().toCharArray();
    }
}
\$\endgroup\$
1
\$\begingroup\$

Your code is, in my opinion, well-written. It is readable and to the point. There a few things that I would like to point out that would fix a few bugs and help you improve your style:

1. Faulty Argument Checking

For your minimal argument checking, you simply detect if the first argument contains "help" anywhere. Therefore, it would not be able to process files like helpme.txt, whelp.xyz, helpless, or any file containing the characters of "help" in that order.

I would suggest an alternative like checking for exactly the word help or counting the number of arguments (when an incorrect number of arguments is presented.

In addition to that, the program should not continue if it detects that a filename was not presented. Or if it should continue, maybe prompt the user for input?

2. Simplification of Mapping Loop

You are extracting the value already present in the map against the given character to an Integer object. Then while reassigning the frequency, you are incrementing it with a prefix operator. I would discourage against that because you will only need the next integer in this instance and there is no need to reassign the variable.

You are using a traditional indexed for-loop which

There exists a method called getOrDefault available for Maps, which we can use get rid of the if statement completely and use a single put statement.

for (char key : chars) {
    int frequency = map.getOrDefault(key, 0);
    map.put(key, frequency + 1);
}

3. Readable printing statement

If you are using Java 8 or above, use the functional programming features to make your code less verbose and more readable. I recommend using the forEach method.

4. Assign consistent names

When someone reads the method's name: fileToCharList, they will expect the return type to be a java.util.List, or one of its subclasses (like ArrayList, LinkedList, etc). If you want to return an array, rename the method fileToCharArray or simply fileToChars. This will help remove any possible ambiguity.

5. Simplifying the reading loop

Since all you do is read characters, there does not seem to be any special reason to proceed line-by-line. You will only skip over new lines with this approach. Instead, you could simply use a FileReader and keep on reading until EOF is reached (i.e. -1 is returned).

The suggested update

Feel free to incorporate as much or as little of the changes I suggested. Here is the updated code for the complete picture:

import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.util.Map;
import java.util.HashMap;

public class Main {
    public static void main(String[] args) {
        if (args[0].contains("help")) {
            System.out.println("usage: java Main filename.txt");
            return; // No need to continue
        }

        Map<Character, Integer> map = new HashMap<>();
        char[] chars = fileToChars(args[0]);
        for (char key : chars) {
            int frequency = map.getOrDefault(key, 0);
            map.put(key, frequency + 1);
        }

        map.forEach((key, value) -> System.out.println(key + ": " + value));
    }

    private static char[] fileToChars(String fileName) {
        File file = new File(fileName);
        if (!file.exists()) {
            // Maybe do something here?
        }
        StringBuilder builder = new StringBuilder();
        try (FileReader reader = new FileReader(file)) {
            int input;
            while ((input = reader.read()) != -1) {
                // If you don't want to append certain characters
                // filter them out here.
                builder.append((char) input);
            }
        } catch (IOException ioe) {
            ioe.printStackTrace();
        }
        return builder.toString().toCharArray();
    }
}

Update: Using ArrayList instead of StringBuilder

As tinstaafl pointed out, using an ArrayList would lead to simpler code and better performance for larger files, I decided to modify the code again:

// imports

public class Main {
    public static void main(String[] args) {
        // Same until this point

        Map<Character, Integer> map = new HashMap<>();
        List<Character> chars = fileToCharList(args[0]);
        for (char key : chars) {
            int frequency = map.getOrDefault(key, 0);
            map.put(key, frequency + 1);
        }

        map.forEach((key, value) -> System.out.println(key + ": " + value));
    }

    private static List<Character> fileToCharList(String fileName) {
        File file = new File(fileName);
        if (!file.exists()) {
            // Maybe do something here?
        }
        List<Character> list = new ArrayList<>();
        try (FileReader reader = new FileReader(file)) {
            int input;
            while ((input = reader.read()) != -1) {
                // If you don't want to append certain characters
                // filter them out here.
                list.add((char) input);
            }
        } catch (IOException ioe) {
            ioe.printStackTrace();
        }
        return list;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Instead of returning a char[] in fileToChars, wouldn't it be better to use an arraylist, to store the characters directly, instead of converting the StringBuilder twice \$\endgroup\$ – tinstaafl Jan 1 at 16:12
  • \$\begingroup\$ @tinstaafl indeed. Using an ArrayList would simplify the code inside the method and retain the name that the OP originally used. I decided to stick to StringBuilder because it was in the original code. There shouldn't be any noticeable performance difference though. \$\endgroup\$ – Hungry Blue Dev Jan 1 at 16:30
  • \$\begingroup\$ If the idea of your code is to show an improvement, it doesn't really make much sense to keep unimproved code there. the performance difference wouldn't be noticeable on relatively small files, but larger files wouldn't definitely show a difference. \$\endgroup\$ – tinstaafl Jan 1 at 16:53
  • 1
    \$\begingroup\$ Regarding point 2: Map has even more very practical methods. Using .merge() inside the loop would simplify it even more: map.merge(key, 1, f -> f + 1); This sets the value to 1 if no value has been assigned yet or adds one to the existing value. \$\endgroup\$ – RoToRa Jan 2 at 15:41
2
\$\begingroup\$

A couple of things I noticed:

Using a TreeMap, instead of converting a StringBuilder to a char[] and creating the map, is much more efficient.

You don't test for an empty argument list.

Telling the user they have to use .txt in the filename is inaccurate as your code doesn't test for that.

When you're printing multiple values on one line, it is more efficient and easier to maintain to use printf, rather than concatenating strings;

Something like this will work:

public static void main(String[] args) throws IOException {
    if (args.length < 1 || args[0].equals("help")) {
        System.out.println("usage: java Main filename");
        return;
    }
    fileToChars(args[0])
            .forEach((k, v) -> System.out.printf("%1$s: %2$d\n", k, v));
}

private static TreeMap<Character, Integer> fileToChars(String fileName) throws IOException {
    final char EOF = (char) -1;
    File file = new File(fileName);
    TreeMap<Character, Integer> builder = new TreeMap<>();
    try (FileReader reader = new FileReader(file)) {
        Character input;
        while ((input = (char) reader.read()) != EOF) {
            Integer count = builder.getOrDefault(input, 0);
            builder.put(input, ++count);
        }
    } catch (IOException e) {
        throw new IOException("Problem reading file");
    }
    return builder;
}

This gives a sorted output of the mapped Characters. If an 'as is' output is wanted, change the TreeMap to a HashMap.

\$\endgroup\$
1
\$\begingroup\$

In addition to the other two answers, I would like to add this:

Prefer 'streaming' processing whenever possible.

Currently, you read the entire file to memory and even operate on it.

builder.toString().toCharArray() will even make the file be in memory twice, once as String, once as char[].

You could also process it line-by-line (or chunk-by-chunk, as you already use a BufferedReader. Have subresults returned and add it to the total result. That way you are far more memory-efficient.

Using as much standard Java functionality

Or, using the java 8 streams, for example:

    Map<Character, Long> frequencyMap = 
    Files.lines(Paths.get(filename))                   //read the lines to a stream
         .flatMap(s-> s.chars().mapToObj(c->(char)c))  //convert string stream to char stream
         .collect(
                 Collectors.groupingBy(                //collect to a map,
                         Function.identity(),          //key is the char
                         Collectors.counting()));      //value is the count

If you need a sorted end-result, sort after all the heavy lifting is done, this is better performance-wise:

   SortedMap<Characted, Long> sortedFrequencyMap = 
                              new TreeMap<Character, Long>(frequencyMap);
\$\endgroup\$

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.