24
\$\begingroup\$

I'm working on a small program to perform various tasks on text content, primarily at the word level. I wrote these methods as ways to help prepare the raw text file into something more malleable, like a List<String>, where at a later point I can perform various routines such as counting and sorting words, and so on.

Concerns:

  • In splitTextStringIntoWordList I found myself having to split textString param into String[] array, and immediately afterwards adding the elements from the array one at a time, parsing with regex, into a List<String>. Is there a better way to do this that might not need as much manipulation?

  • Are some of my methods just doing too many things between parameters and return?

  • Is the JavaDoc clear, concise and descriptive?

  • What are some beginner mistakes I might be making?

import java.util.Arrays;
import java.util.ArrayList;
import java.util.List;
import java.util.Collections;
import java.util.regex.*;
import java.net.URL;
import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.io.IOException;

public class TextFileWordSplitter {

    /**
     * Remove punctuation marks from a String using regular expressions.
     * <p>
     *     This will account for contractions such as "it's" and "can't", as well as
     *     hyphenated words such as "first-class" and "low-budget", which in both cases
     *     will be considered as whole words.
     * </p>
     * @param input  The String from which to remove punctuation
     * @return  The String with the punctuation removed, or empty String
     */
    static String removePunctuationFromString(String input) {

        Pattern regex = Pattern.compile("([A-Za-z]?[\\-']?[A-Za-z])+");
        Matcher matcher = regex.matcher(input);
        if (matcher.find()) {
            return matcher.group();
        } else {
            return "";
        }
    }

    /**
     * Create a String by fetching a text file at the provided URL.
     * @param url  The URL where the text file is located.
     * @return  The content of the text file, or null
     * @throws IOException
     */
    static String readUrlTextContent(String url) throws IOException {

        URL source = new URL(url);
        try (BufferedReader reader = new BufferedReader(new InputStreamReader(source.openStream()))) {
            StringBuilder builder = new StringBuilder();
            String line = reader.readLine();

            while (line != null) {
                builder.append(line);
                builder.append("\n");
                line = reader.readLine();
            }
            return builder.toString();
        } catch (IOException e) {
            e.printStackTrace();
        }
        return null;
    }

    /**
     * Split a String into an ArrayList of individual words as separate elements.
     * <p>
     *     The words are all converted to uppercase, such that "Hello", "hello" and "HELLO"
     *     will all become the same word string, "HELLO".
     * </p>
     * @param textString  The String which is intended to be split into a list of words
     * @return  An ArrayList containing one word per element, or null
     */
    static List<String> splitTextStringIntoWordList (String textString) {

        try {
            String allWhiteSpace = "\\s+";
            String[] splitText = textString.toUpperCase().split(allWhiteSpace);
            List<String> wordList = new ArrayList();
            for (String word : splitText) {
                Collections.addAll(wordList, removePunctuationFromString(word));
            }
            wordList.removeAll(Arrays.asList("", null));
            return wordList;
        } catch (Exception e) {
            e.printStackTrace();
        }
        return null;
    }

    public static void main(String[] args) {

        String textString = null;
        try {
            textString = readUrlTextContent("http://textfiles.com/stories/antcrick.txt");
        } catch (IOException e) {
            e.printStackTrace();
        }

        List<String> wordList = splitTextStringIntoWordList(textString);

        /* Print each word along with its index */
        int wordIndex = 0;
        for (String word : wordList) {
            System.out.println("[" + wordIndex++ + "] " + word);
        }
    }
}

The output of the program, using The Ant and the Cricket as a source, is as follows:

[0] THE
[1] ANT
[2] AND
[3] THE
[4] CRICKET
[5] ONCE
[6] UPON
[7] A
[8] TIME
...
[368] WELL
[369] TRY
[370] DANCING
[371] NOW

Complete output on PasteBin

\$\endgroup\$
15
\$\begingroup\$

Problems

I'm not sure that TextFileWordSplitter is a good name for the class, especially since the source of the text is, in general, a network resource rather than a java.io.File.

List<String> wordList = new ArrayList(); should be List<String> wordList = new ArrayList<>(); to suppress a compiler warning.

The readUrlTextContent() function declares that it throws IOException, but instead it catches almost every IOException and returns null. (The only IOException that can get thrown is a MalformedURLException.) You should just let all IOExceptions propagate naturally.

Make up your mind which functions are public and which ones are private. The default access is almost never a good choice.

Nitpicks

Your regex does not need the capturing parentheses. It also does not need the backslash to quote the hyphen in the character class, since the hyphen it taken literally if it is the first or the last character in the character class.

The JavaDoc should avoid documenting implementation details such as "using regular expressions" — that's none of the caller's business, unless you want to also document exactly what regex is used. JavaDoc should be written in the third-person indicative mood rather than the imperative.

Decomposition

I like that you have split up the work into functions, but I would decompose the work differently.

Having the .toUpperCase() call buried within splitTextStringIntoWordList() is surprising. What does the uppercase transformation have to do with splitting? I would move it into the removePunctuationFromString() function and rename the function to normalizeWord().

readUrlTextContent() is probably harmful. This task can be accomplished without random access to the stream, so you don't need to buffer the entire text into a string. Just let the BufferedReader do its job: it will buffer just enough to enhance performance, and discard the parts of the text that you have already processed.

Instead of calling String.split(), I recommend using a Scanner, which is a convenient way to fetch a word at a time. I would define two versions of the words() function: one that accepts a Scanner, and another that accepts a URL.

import java.io.*;
import java.net.URL;
import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class WordExtractor {
    private static final Pattern WORD_PATTERN =
        Pattern.compile("[A-Za-z]*[-']?[A-Za-z]+");

    /**
     * Extracts an alphabetic word, possibly containing up to one hyphen or
     * apostrophe, and returns it in uppercase.
     *
     * @return The extracted word, or an empty string if the input is all
     *         punctuation.
     */
    private static String normalizeWord(String s) {
        Matcher m = WORD_PATTERN.matcher(s);
        return m.find() ? m.group().toUpperCase() : "";
    }

    public static List<String> words(URL url) throws IOException {
        try ( InputStream is = url.openStream();
              BufferedInputStream bis = new BufferedInputStream(is);
              Scanner scanner = new Scanner(bis) ) {
            return words(scanner);
        }
    }

    public static List<String> words(Scanner scanner) {
        List<String> results = new ArrayList<>();
        scanner.reset();
        while (scanner.hasNext()) {
            String word = normalizeWord(scanner.next());
            if (!word.isEmpty()) {
                results.add(word);
            }
        }
        return results;
    }

    public static void main(String[] args) throws IOException {
        URL url = new URL("http://textfiles.com/stories/antcrick.txt");
        int i = 0;
        for (String word : words(url)) {
            System.out.printf("[%d]: %s\n", i++, word);
        }
    }
}
\$\endgroup\$
14
\$\begingroup\$

In splitTextStringIntoWordList I found myself having to split textString param into String[] array, and immediately afterwards adding the elements from the array one at a time, parsing with regex, into a List<String>. Is there a better way to do this that might not need as much manipulation?

Wells, in Java 8...

List<String> result = Pattern.compile(allWhitespace).splitAsStream(textString.toUpperCase())
                                .map(TextFileWordSplitter::removePunctuationFromString)
                                .collect(Collectors.toList());

I'm also not quite sure if you need the try { } catch (Exception e) { } statements or not... generally, one should be more particular about the types of checked Exceptions, instead of the all-encompassing Exception. If there is no checked Exception to catch for, then it may be better to just remove the try-catch statement, unless there are specified runtime Exception that requires special handling (e.g. prompting user in case of NullPointerException, something like that).

Still on a Java 8-oriented suggestion, a stream-oriented way of reading from a BufferedReader can be the lines() method (edit: as pointed out by @200_success and @Boris the Spider, there is probably a better name/method argument to use here I have updated to use a URL argument instead):

private static List<String> parseContent(URL url) {
    try (Stream<String> lines = new BufferedReader(
                    new InputStreamReader(url.openStream())).lines()) {
        return lines.flatMap(Pattern.compile(allWhitespace)::splitAsStream)
                .map(TextFileWordSplitter::removePunctuationFromString)
                .map(String::toUpperCase)
                .collect(Collectors.toList());
    } catch (IOException e) {
        throw new UncheckedIOException(e);
    }
}

Over here, flatMap() is used to 'convert' each stream (i.e. line) element into individual streams of words, and these are appended together. All the words will then be mapped with your TextFileWordSplitter::removePunctuationFromString method reference.

\$\endgroup\$
  • 1
    \$\begingroup\$ A function with the signature parseContent(String) has a good chance of being mistakenly called with the text content rather than the URL as an argument. Better to have it accept a URL instead of a String so that such a mistake can't happen. \$\endgroup\$ – 200_success Sep 3 '15 at 10:03
  • \$\begingroup\$ @200_success certainly, have edited my answer to include that info tidbit. :) \$\endgroup\$ – h.j.k. Sep 3 '15 at 10:10
  • 1
    \$\begingroup\$ @h.j.k. I think what 200_success is saying is the the method should accept a URL rather than a String to avoid confusion. In general, using method argument types to document required input is a good idea. \$\endgroup\$ – Boris the Spider Sep 4 '15 at 9:00
  • \$\begingroup\$ @BoristheSpider gotcha. :) \$\endgroup\$ – h.j.k. Sep 4 '15 at 9:04
12
\$\begingroup\$

Have you heard about Turkey?

Try inserting the following line at the top of your main:

java.util.Locale.setDefault(java.util.Locale.forLanguageTag("tr-TR"));

Now you output will look a little different:

[0] THE
[1] ANT
[2] AND
[3] THE
[4] CRICKET
[5] ONCE
[6] UPON
[7] A
[8] T
...
[351] WELL
[352] TRY
[353] DANC
[354] NOW

In the turkish locale "i".toUpperCase(); is İ (U+0130 "Latin Capital Letter I With Dot Above").

When manipulating strings for any purpose other than display you should always specify a Locale. It's even better if you never use toUpper or toLower and instead normalize first and then use case folding or case insensitive comparisons when needed.

Similarly in your regex you use [A-Za-z] which doesn't handle Unicode letters. You could use [\p{L}] to get all letters instead. See Unicode Support under Pattern javadoc for additional ways to handle Unicode in Java regex.

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

A few things:


    if (matcher.find()) {
        return matcher.group();
    } else {
        return "";
    }

You could swap this with a ternary:

return matcher.find() ? matcher.group() : "";

You could consider adding the case insensitive flag to your pattern to avoid blocks like a-zA-Z

Pattern.compile("([A-Za-z]?[\\-']?[A-Za-z])+");

Which could be improved to:

([a-z]?['-]?[a-z])+

with the i flag, which would be Pattern.CASE_INSENSITIVE or (?i). See this regex101.com link for an example.

\$\endgroup\$
  • 3
    \$\begingroup\$ Javadoc on the case-insensitive flag: "Specifying this flag may impose a slight performance penalty." (just a note for future reference) \$\endgroup\$ – h.j.k. Sep 3 '15 at 10:44
  • 1
    \$\begingroup\$ What about: regex101.com/r/tR1oN4/2 ? (finishes in 1179 steps vs. 2895 steps) \$\endgroup\$ – Ismael Miguel Sep 3 '15 at 11:39
  • 1
    \$\begingroup\$ Post it as your answer \$\endgroup\$ – Quill Sep 3 '15 at 12:19

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.