5
\$\begingroup\$

As I'm doing the MOOC Java course I decided to write a small program to count the word occurrences that accepts as the first (mandatory) input a path to a text file (extension not enforced) and as many words you want to check (optional). It prints out the results to the terminal. Checked with books in .txt format from Project Gutenberg.

This is my first ever Java program, thank you.

package com.my.name;

public class Main {

    public static void main(String[] args) {
        if (args.length == 0) {
            throw new IllegalArgumentException("Required file name");
        }

        Option option = new Option();


        if (args.length > 1) {
            for (int i = 1; i < args.length; i++) {
                option.addWords(args[i]);
            }
        }

        Stats stats = new Stats(option);
        String fileName = args[0];


        TextFileReader textReader = new TextFileReader(fileName, stats);
        textReader.readTest();

        stats.printOccurrences();
    }
}
package com.my.name;

import java.util.ArrayList;
import java.util.Locale;

public class Option {
    private final ArrayList<String> words;

    public Option() {
        this.words = new ArrayList<>();
    }

    public void addWords(String word) {
        this.words.add(word.toLowerCase(Locale.ROOT));
    }

    public boolean hasFilters() {
        return !(this.words.size() == 0);
    }

    public boolean hasWord(String word) {
        return this.words.contains(word);
    }
}
package com.my.name;

import java.io.BufferedReader;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Locale;
import java.util.regex.Pattern;

public class TextFileReader {

    private final String fileName;
    private final Stats stats;

    public TextFileReader(String fileName, Stats stats) {
        this.fileName = fileName;
        this.stats = stats;
    }

    public void readTest() {
        try (BufferedReader reader = Files.newBufferedReader(Paths.get(this.fileName))) {
            String line;
            Pattern pattern = Pattern.compile("[^a-zA-Z]"); // @TODO if needed
            while ((line = reader.readLine()) != null) {
                String[] words = line.split(" ");
                for (String word : words) {
                    word = pattern.matcher(word).replaceAll("").toLowerCase(Locale.ROOT);
                    if (word.isBlank()) continue;
                    this.stats.mapWordToCount(word);
                }
            }
        } catch (IOException e) {
            System.out.println(e.getMessage());
        }
    }
}

package com.my.name;

import java.util.Comparator;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.stream.Collectors;

public class Stats {
    private final Map<String, Integer> wordCounts;
    private final Option option;
    private int numberOfWords;

    public Stats(Option option) {
        this.wordCounts = new HashMap<>();
        this.option = option;
        this.numberOfWords = 0;
    }

    public void mapWordToCount(String word) {
        this.numberOfWords++;
        this.wordCounts.merge(word, 1, Integer::sum);
    }

    public void printOccurrences() {
        boolean hasFilters = option.hasFilters();
        Map<String, Integer> topTen = this.wordCounts
                .entrySet()
                .stream()
                .filter(w -> !hasFilters || option.hasWord(w.getKey()))
                .sorted(Map.Entry.comparingByValue(Comparator.reverseOrder()))
                .limit(20L)
                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (e1, e2) -> e1, LinkedHashMap::new));

        for (String word : topTen.keySet()) {
            System.out.println("Word " + " | " + "\u001B[31m" + word + "\u001B[0m" + " | " + " occurred " + wordCounts.get(word) + " times");
        }

        System.out.println("Total words: " + this.numberOfWords);
    }
}

Thanks again

\$\endgroup\$
0
5
\$\begingroup\$

Unnecessary conditional

        if (args.length > 1) {
            for (int i = 1; i < args.length; i++) {
                option.addWords(args[i]);
            }
        }

If args.length is not greater than one, then for (int i=0; i < args.length; i++) will execute zero times. There is no need to protect the for loop with an if statement. The following would work just as well:

        for (int i = 1; i < args.length; i++) {
            option.addWords(args[i]);
        }

Underhanded return

        TextFileReader textReader = new TextFileReader(fileName, stats);
        textReader.readTest();

The TextFileReader is only used once, immediately after construction. The above two statements could be simplified to:

        new TextFileReader(fileName, stats).readTest();

Notice that nothing is returned from readTest(). It is stranger for a reader to not return what it reads. In fact, the "return value" structure (stats) is passed into the constructor.

It would make more sense to pass the structure to be filled in into the function that does the reading. Then the class wouldn't need to store it as a member variable. Actually, the fileName is passed to the constructor, and only used int the testRead function as well. This could also be passed to the read function, making the TextFileReader object unnecessary; a static method could be used:

public class TextFileReader {
    public static void readTest(String fileName, Stats stats) {
        try (BufferedReader reader = Files.newBufferedReader(Paths.get(fileName))) {
            String line;
            Pattern pattern = Pattern.compile("[^a-zA-Z]");
            while ((line = reader.readLine()) != null) {
                String[] words = line.split(" ");
                for (String word : words) {
                    word = pattern.matcher(word).replaceAll("").toLowerCase(Locale.ROOT);
                    if (word.isBlank()) continue;
                    stats.mapWordToCount(word);
                }
            }
        } catch (IOException e) {
            System.out.println(e.getMessage());
        }
    }
}

It would be used with:

        TextFileReader.readTest(fileName, stats);

Naming

Read Test

Why readTest()? Is it really a test? Should this not be readText()? Or readStats() or readWordCounts()?

Add Words

    public void addWords(String word) {
        this.words.add(word.toLowerCase(Locale.ROOT));
    }

Doesn't exactly look like it is adding words; at best it is adding a word. Maybe addWord(String word) would be better?

Option

The Option class, on the other hand, stores many words. Perhaps Options would be better, to indicated it holds more than one option.

Does it hold options, though? Or does it store Words?

Top Ten

Why does the list of top 10 words by occurrence have 20 entries?

Efficiency

Counts of all words

If you run this with a large tome - say "War & Peace", and you are counting the number of times the words "France" and "Russia" occur, you will build up a Map<String, Integer> wordCounts for every word that occurs in the text, most of which are never needed.

You can't change which words are reported by Stats. So, why counts of all words which are encountered?

    public void mapWordToCount(String word) {
        this.numberOfWords++;
        if (!option.hasFilters() || option.hasWord(word) {
            this.wordCounts.merge(word, 1, Integer::sum);
        }
    }

... and remove the filter from printOccurences().

Filtering

Option.hasWord() will go through the ArrayList<String> words one word at a time, searching for a match. Using a Set<String> would be more efficient.

Filtering

Conditional filtering in the stream results in an unnecessary stage, and slower streaming. An if(option.hasFilters()) { ... with filter ... } else { ... without filter } construct would remove the !hasFilters test from every entry.

Ok, while "War and Peace" has a half-million words, it only a few thousand unique words, so it may not be a serious performance issue here. Still, it isn't that hard to construct the stream conditionally:

        var stream = this.wordCounts.entrySet().stream();

        // Only add the filter step if filtering is needed.
        if (hasFilters)
            stream = stream.filter(w -> option.hasWord(w.getKey()));

        Map<String, Integer> topTen = stream
                .sorted(Map.Entry.comparingByValue(Comparator.reverseOrder()))
                .limit(20L)
                .collect(Collectors.toMap(Map.Entry::getKey,
                                          Map.Entry::getValue,
                                          (e1, e2) -> e1,
                                          LinkedHashMap::new));

If you're using Java 9 or earlier, you won't have the var keyword and you'll need to use the exact type:

Stream<Map.Entry<String, Integer>> stream = this.wordCounts.entrySet().stream();

Organization

You should separate printing of the occurrences from determining the filtered, sorted list of top word occurrences.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks a lot for the review. Didn't know that you can create a stream with some options and then adding others and in the end consume it. Totally makes sense what you said about naming, (test/text I always type it wrong!). I chose the singular Option because I thought in Java classes should be singularly named? \$\endgroup\$
    – Alex.B
    Jan 5 at 9:18
  • 1
    \$\begingroup\$ The Stream class: "A stream pipeline consists of a source ..., zero or more intermediate operations (which transform a stream into another stream, such as filter(Predicate)), and a terminal operation ... . Streams are lazy; computation on the source data is only performed when the terminal operation is initiated, and source elements are consumed only as needed." Until the terminal operation, a stream is just an object that more intermediate operations can be applied to, creating new stream objects. \$\endgroup\$
    – AJNeufeld
    Jan 5 at 14:38
  • 1
    \$\begingroup\$ Java classes are often named singular because they define one type of object, of which multiple instances may be created. Containers are singular because a bag is one object, even if you put multiple objects in it. But if you have a class where each instances represents multiple elements, using a plural name for the class scans better. Also, Java has several plural named classes. You even used one, Collectors, but there are others like Collections, Arrays, and SocketOptions. \$\endgroup\$
    – AJNeufeld
    Jan 5 at 14:50
5
\$\begingroup\$

When structuring your code to components (classes) you should think about what the application tries to accomplish and what steps it needs to do to achieve that. In this example, the steps are quite clear:

  1. Read lines
  2. Split line to words
  3. Normalize words to lower case
  4. Filter interesting words
  5. Gather statistics
  6. Print statistics

These are the responsibilities of your application. Implementation of each one should go to it's own class. This is called single responsibility principle.

Reading lines provides a set of strings. This is all mundane stuff already provided by the JRE. The files can be big so this seems like a place where Streams would be handy, so use BufferedReader.lines(). What you missed in your code is handling the input text character set correctly. If you are from the US your program will most likely fail a bit if it encounters the Finnish epic Kalevala. When you work with text files you ALWAYS have to provide the character encoding. Relying on the system default will cause bugs at some point.

Splitting a line into words provides a stream of words from a String. Stream.flatMap() should be used for this with a class implementing Function<String, Stream<String>>.

Then, if you want to, you can normalize the words to lower case using a Function<String, String>. Again, you need to specify a locale that can handle the characters in the text.

Once you have a stream of words, filtering becomes a trivial set of classes that implement Predicate<String>.

Counting the words that are left after the filtering is an operation where a grouping collector would come in handy.

Printing statistics is an UI operation and should be separated from the statistics. It's a class or static method that accepts a Map<String, Integer> as a parameter and does whatever it needs with it.

Once you have split te code to reasonably small parts you combine everything into a simple stream operation:

Map<String, Integer> occurrences = reader
    .lines()
    .flatMap(new LineTokenizer())
    .map(new WordNormalizer())
    .filter(...)
    .collect(...);

Note that your implementation breaks hyphenated words, like mother-in-law and none of the examples that process the input as lines can correctly handle hyphenated words that are broken to two lines without becoming overly complicated.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks Torben. Didn't know about BufferedReader.lines(), I thought reader.readLine() is a stream as well. Better get back and experiment more. Cheers! \$\endgroup\$
    – Alex.B
    Jan 10 at 14:07

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.