2
\$\begingroup\$

I wrote implementation of unix console program word count "wc" in Java. Please evaluate it. Is there any better solution?

import java.util.ArrayList;
import java.io.*;
import java.util.HashMap;
import java.util.Map;

enum CountMode {
    LINES,
    BYTES,
    SYMBOLS,
    LONGEST_LENGTH,
    WORDS,
    DEFAULT
}

enum ReadMode {
    FILE,
    CONSOLE
}

class Counter {
    private CountMode countMode;
    private ReadMode readMode;
    private ArrayList<File> files;

    Counter(CountMode countMode, ReadMode readMode, ArrayList<File> files) {
        this.countMode = countMode;
        this.readMode = readMode;
        this.files = files;
    }

    public void process() {
        HashMap<File, ArrayList<String>> input = getInput();
        HashMap<File, Integer> result = new HashMap<>();
        for (Map.Entry<File, ArrayList<String>> pair : input.entrySet()) {
            switch (countMode) {
                case LINES:
                    result.put(pair.getKey(), countLines(pair.getValue()));
                    break;
                case BYTES:
                    result.put(pair.getKey(), countBytes(pair.getValue()));
                    break;
                case SYMBOLS:
                    result.put(pair.getKey(), countSymbols(pair.getValue()));
                    break;
                case LONGEST_LENGTH:
                    result.put(pair.getKey(), getLongestLength(pair.getValue()));
                    break;
                case WORDS:
                    result.put(pair.getKey(), countWords(pair.getValue()));
                    break;
                case DEFAULT:
                    result.put(pair.getKey(), countWords(pair.getValue()));
                    break;
            }
        }
        for (Map.Entry<File, Integer> data : result.entrySet()) {
            if (data.getKey() != null) {
                System.out.printf("%d\t%s\n", data.getValue(), data.getKey().getName());
            }
            else {
                System.out.printf("%d\t%s", data.getValue(), "[console]");
            }
        }
    }

    private HashMap<File, ArrayList<String>> getInput() {
        HashMap<File, ArrayList<String>> result = new HashMap<>();
        switch (readMode) {
            case FILE:
                for (File file : files) {
                    try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
                        ArrayList<String> strings = new ArrayList<>();
                        String line;
                        while ((line = bufferedReader.readLine()) != null)
                            strings.add(line);
                        result.put(file, strings);
                    } catch (FileNotFoundException ex) {
                        System.err.println(ex.getMessage());
                    } catch (IOException ex) {
                        System.err.println("IO exception");
                    }
                }
                break;
            case CONSOLE:
                InputStreamReader inputStreamReader = new InputStreamReader(System.in);
                BufferedReader bufferedReader = new BufferedReader(inputStreamReader);
                ArrayList<String> strings = new ArrayList<>();
                String line;
                try {
                    while ((line = bufferedReader.readLine()) != null)
                        strings.add(line);
                } catch (IOException ex) {
                    System.err.println(ex.getMessage());
                }
                result.put(null, strings);
                break;
        }
        return result;
    }

    private int countLines(ArrayList<String> lines) {
        return lines.size();
    }

    private int countBytes(ArrayList<String> lines) { //support UTF-8 only
        int count = 0;
        for (String line : lines) {
            try {
                byte[] utf8Bytes = line.getBytes("UTF-8");
                count += utf8Bytes.length;
            } catch (UnsupportedEncodingException ex) {
                System.err.println(ex.getMessage());
            }
        }
        return count;
    }

    private int countSymbols(ArrayList<String> lines) {
        int count = 0;
        for (String line : lines) {
            count += line.length();
        }
        return count;
    }

    private int getLongestLength(ArrayList<String> lines) {
        int longest = -1;
        for (String line : lines) {
            if (line.length() > longest)
                longest = line.length();
        }
        return longest;
    }

    private int countWords(ArrayList<String> lines) {
        int count = 0;
        for (String line : lines) {
            count += line.split(" ").length;
        }
        return count;
    }

}

public class Main {
    public static void main(String[] args) {
        CountMode countMode = CountMode.DEFAULT;
        ReadMode readMode = ReadMode.CONSOLE;
        ArrayList<File> files = new ArrayList<>();
        if (args.length > 0) {
            switch (args[0]) {
                case "-l":
                    countMode = CountMode.LINES;
                    break;
                case "-c":
                    countMode = CountMode.BYTES;
                    break;
                case "-m":
                    countMode = CountMode.SYMBOLS;
                    break;
                case "-L":
                    countMode = CountMode.LONGEST_LENGTH;
                    break;
                case "-w":
                    countMode = CountMode.WORDS;
                    break;
            }
        }
        for (String fileName : args) {
            File file = new File(fileName);
            if (file.exists() && !file.isDirectory()) {
                files.add(file);
                readMode = ReadMode.FILE;
            }
        }
        Counter counter = new Counter(countMode, readMode, files);
        counter.process();
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ For people who aren't familiar with the wc program, can you please provide a brief summary of what the program is supposed to do, expected inputs/outputs, etc? \$\endgroup\$ Commented May 29, 2018 at 20:02
  • 1
    \$\begingroup\$ @Dannnno as usual for *nix commands, man wc gives you all the info (try google it) \$\endgroup\$ Commented May 30, 2018 at 7:02
  • \$\begingroup\$ Have you seen the source code of the "wc" program? gnu.org/software/cflow/manual/html_node/… \$\endgroup\$ Commented May 30, 2018 at 12:24
  • \$\begingroup\$ @SharonBenAsher and as usual for Code Review, if I have to go further than the body of the question for context/understanding I'll leave a comment/VTC as lacking context. \$\endgroup\$ Commented May 30, 2018 at 13:59

1 Answer 1

3
\$\begingroup\$

Here are my comments in order of severity

1) Bugs

1.1) Split into words

in countWords() you split the line by a single space character. That has two problems: first, it will split two consecutive spaces, adding an "empty" String to the output array (try it), and second, it ignores all other words delimiters (like tab and new line characters). luckily for you, regex has a predefined character classes that defines all white space characters. this regex "\\s+" will split theline by one or more white space characters. It will correctly split "Two Words" into two words (you can try this on single space regex and see the difference). However, it will not split "Two-Words" or "Two(Words)" so you may need to further enhance the regex pattern (depending on wc specifications)

1.2) resource leak

You get a plus for using try-with-resources when reading from files. However, why did you not use the same construct when reading from the console? you should know that opening multiple readers from the console costs OS resources.

While we are discussing getInput(), you should be aware that Java 7 gives you a method that allows you to read whole files in one line of code: Files.readAllLines()

2) Performance

2.1) Greedy load of files

You first read all the files into memory. That may pose a problem in memory consumption if you are given many files and/or big ones. The processing doesn't require loading all files upfront. A better design would be to read the input line-by-line and do the counting on this line. This way there is only one line in memory at any given time.

3) Design

3.1) Modularity

You should follow the design rule of "Code to the Interface". Almost all of your methods break this rule.
Instead of specifying concrete implementations as arguments and/or return value:
private HashMap<File, ArrayList<String>> getInput()
Specify the interface:
private Map<File, List<String>> getInput()
This signature allows the method to decide if it wants to return ArrayList or LinkedList and same for Map implementation.

3.2) Extensibility

You get a plus for using enum instead of String. and the splitting of counting into separate methods is another plus. However, we can go further with the extensibility concern. Currently if you want to add more counting capabilities, you will need to add a counting method, an enum value, and a case clause to the switch statement. What you can do is put each counting method in its own class that implement the (new) CountLine interface

interface CountLine {
    int count(String line) throws Exception;
}

(this is following the performance advice regarding processing of sinlge lines)
and you wil have several implementations like

class CountLines implements CountLine {
    @Override
    public int count(String line) throws Exception {
        return 1;
    }
}
class CountBytes implements CountLine {
    @Override
    public int count(String line) throws Exception {
        return line.getBytes("UTF-8").length;
    }
}

now what can be done is associate a CountLine implementation with an enum

enum CountMode {
    LINES (new CountLines()),
    BYTES (new CountBytes()),
    ...

    public CountLine getCounter() {
        return counter;
    }
    private CountLine counter;

    private CountMode(CountLine counter) {
        this.counter = counter;
    }
}

this elimiates the switc statement. you can get a CountLine instance directly from the CountMode argument.

3.3) Make use of Lambda expr

Looking at CountLine we see that this fits the Functional Interface definition. This means that instead of having to write a separate implementation class, we can write a Lambda expr as the enum argument:

enum CountMode {
    LINES ((line) -> 1),
    BYTES ((line) -> line.getBytes("UTF-8").length),

and there you have it: the enum and its counting implementation in one line. This will not always work. For example, with longest length you need to keep state between invocations so you still need an instance of custom class.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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