This is a simple word frequency counter in Java. I am just wondering if I can improve or compact this code better. You choose a file to be counted with a JFileChooser, and the results get outputted into a file. The biggest struggle I had was sorting the HashMap by values. I got it to work but I feel like it should be easier to sort. I'm also wondering if I should break up my code in main into more methods. I saw a Python script of this that does the same thing but in half the lines of code. Is that just Java or does my code need touching up?

import java.io.BufferedWriter;
import java.io.File;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;

import javax.swing.JFileChooser;

public class FrequencyCounter {

    public static void main(String[] args) throws IOException {
        JFileChooser fc = new JFileChooser();
        File file = null;
        fc.setCurrentDirectory(new File(System.getProperty("user.dir")));
        if (fc.showOpenDialog(null) == JFileChooser.APPROVE_OPTION) {
            file = fc.getSelectedFile();
        }
        FileReader is = new FileReader(file);
        char[] scriptBytes = new char[(int) file.length()];
        is.read(scriptBytes);
        is.close();
        String script = new String(scriptBytes);
        String[] words = script.split("[ \\r?\\n\\-]");
        List<String> wordList = new ArrayList<>(Arrays.asList(words));
        wordList = cleaner(wordList);
        HashMap<String, Integer> counter = new HashMap<>();
        for (String word : wordList) {
            if (counter.containsKey(word))
                counter.put(word, counter.get(word) + 1);
            else
                counter.put(word, 1);
        }
        counter = entriesSortedByValues(counter);
        BufferedWriter writer = new BufferedWriter(new FileWriter(new File(file.getAbsolutePath().replace(file.getName(), "") + "result_" + file.getName())));
        for (Entry<String, Integer> ent : counter.entrySet()) {
            writer.write(ent.getKey() + " " + ent.getValue());
            writer.newLine();
        }
        writer.close();
    }

    private static List<String> cleaner(List<String> words) {
        for (int i = 0; i < words.size(); i++) {
            words.set(i, words.get(i).replaceAll("[1234567—890!@#$%^&*()_+|\\-=~`{}\\[\\]:;\"<>,.?/]","").toLowerCase());
        }
        words.removeAll(Arrays.asList(""));
        return words;
    }

    private static <K,V extends Comparable<? super V>> 
        HashMap<K, V> entriesSortedByValues(Map<K,V> map) {

        List<Entry<K,V>> sortedEntries = new ArrayList<Entry<K,V>>(map.entrySet());

        Collections.sort(sortedEntries, 
            new Comparator<Entry<K,V>>() {
                @Override
                public int compare(Entry<K,V> e1, Entry<K,V> e2) {
                    return e2.getValue().compareTo(e1.getValue());
                }
            }
        );
        HashMap<K,V> result = new LinkedHashMap<K,V>();
        for (Entry<K,V> ent : sortedEntries)
            result.put(ent.getKey(), ent.getValue());

        return result;
    }

}
  • 2
    Java is a verbose language, expect solutions written in it to be longer than in any dynamic language. – yuri May 28 '17 at 6:55
  • Does the Python script have a GUI for choosing files? If not, there's your answer. (I could do a far less efficient version of this in about 3 lines in Java (excluding boilerplate), utilising Java 8 streams) – Tamoghna Chowdhury May 28 '17 at 8:43
  • Normally, you'll want methods outside the main class, they can the be referenced as static methods, also since your methods are in the main class, they don't have to be static – EastXWest May 30 '17 at 6:45
up vote 7 down vote accepted
  1. It's normal for Java programs to be longer. I wouldn't care about the number of lines per se.

  2. The main method does too much stuff:

    • It asks the user to choose the file
    • It reads the content of the file
    • It counts the number of occurrences
    • It prints them to another file

    One method should be responsible for one thing. There're at least 4 different (very different) things here.

    In fact, I believe that the first thing (and probably the second and the last thing, too) doesn't belong in the FrequencyCounter class. Why? It makes sense for the counter to count occurrences and do just it. It'll make it more reusable. Think what happens if you want to change the output format, the way the user choose the file or just use it as part of your different project.

  3. I would redesign the FrequencyCounter it so that it has one method that takes a string and returns a map with different words and the number of their occurrences.

  4. The reading/writing stuff and the user dialog belong to a different class. These are different concerns. So I'd create another class to handle file I/O and one more class to interact with a user as these things have nothing to do together. They can change independently and they don't need each other to work. They're separate entities.

  5. You don't handle the case when the file is null (that is, when the user doesn't choose a file). A NullPointerException isn't very user-friendly.

  6. This piece of code:

     if (counter.containsKey(word))
         counter.put(word, counter.get(word) + 1);
     else
         counter.put(word, 1);
    

    can be simplified to counter.put(counter.getOrDefault(word, 0) + 1); unless you're using an older Java version.

  7. If you plan to work with large files (really large, such that their content doesn't fit into main memory), you'll need to get rid of reading the entire file into a String. A more space-efficient solution would be to read it in chunks and process them separately (the words are normally short, so it can save a lot of memory). Sure, it makes the code more complicated (for instance, you'll need to carefully check when the word ends), so it's worth doing only if you plan to work with large amounts of data.

  8. A method name is normally a verb. cleaner doesn't sound very good to me. I'd rename it to something like cleanWords. I'd also put cleaning one word into a separate method to make it more manageable. You can go even further and put all cleaning stuff into a separate class to increase the flexibility (it also makes sense from a design point of view: cleaning and counting are different responsibilities and they are pretty much independent).

  9. The way you work with the output file is potentially dangerous: if such a file already exists, it'll be overwritten. I don't think that doing it silently is a good idea. There're a few different ways to deal with it: generating a different unique name, aborting the program or letting the user choose the output file. Either of them is better than replacing the content of the file silently.

  • I'm wondering whether the multiple lines of import statements need to be fixed or not?? In another word, should we use import java.util.*; ? – Angela Pan May 31 '17 at 18:58
  • @AngelaPan There's nothing to fix here. Your way is shorter, but it also imports the stuff we don't use. Neither of them is "always right", but I prefer explicit imports. – kraskevich May 31 '17 at 19:33

Picking up the suggestion of @Tamoghna Chowdhury I wrote a sample implementation using Java8 Streams.

Note that the suggestions of @kraskevich aren't considered in my implementation. You should only consider this as a proof of concept of what is possible (in particular reducing the line count) by using the appropriate API of Java.

public class FrequencyCounter {

    private static Map<String, Long> countFrequency(Stream<String> valueStream) {
    return valueStream
            .map(s -> s.split(" "))
            .flatMap(Arrays::stream)
            .map(FrequencyCounter::clean)               
            .filter(s -> !s.equals(""))
            .collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));
    }

    public static void main(String[] args) throws IOException {
        JFileChooser fc = new JFileChooser();
        fc.setCurrentDirectory(new File(System.getProperty("user.dir")));
        if (fc.showOpenDialog(null) == JFileChooser.APPROVE_OPTION) {
            Map<String, Long> map = countFrequency(
                    Files.lines(Paths.get(fc.getSelectedFile().getAbsolutePath())));

            BufferedWriter writer = new BufferedWriter(new FileWriter(new File(fc.getSelectedFile().getAbsolutePath().replace(fc.getSelectedFile().getName(), "") + "result_" + fc.getSelectedFile().getName())));
            for (Entry<String, Long> ent : map.entrySet()) {
                writer.write(ent.getKey() + " " + ent.getValue() + "\n");
                writer.newLine();
            }
            writer.close();
        }
    }

    private static String clean(String word) {
        return word.replaceAll("[1234567—890!@#$%^&*()_+|\\-=~`{}\\[\\]:;\"<>,.?/]", "").toLowerCase(); 
    }
}
  • This only splits by line, not word – gommb May 28 '17 at 18:52
  • You are right. I made an edit adding the necessary split – Tobias Walter May 28 '17 at 19:57

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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