Counting unique words, numbers, or phrases

Question

I was given the below scenario to write a java program by a Software company. I did my best and was well tested as well. They came back rejecting my application without giving any review comments. I would like to know what I did wrong so that I can improve on what is missing.

The test was easy and the code is in the github link. Can someone review my code and point out my mistakes? Proper use of object oriented design to ensure extensibility. As an example, how much would the design have to change if 10 new patterns were to be added?

Evaluation criteria:

• Loose coupling between input source, core logic, and output source.

• Unit tests that test behavior instead of just methods.

• Program should be packaged such that no setup is required to make it run.

Write a program (source and unit tests) "PatternCounter" that reads an input document from a file (path provided as first command line argument) and calculates the number of occurrences of one of the 3 patterns based on the second command line argument:

1: Counts occurrences of each unique word in the document

2: Counts occurrences of each unique number in the document

3: Counts occurrences of each unique phrase of three consecutive words in the document

It then prints each word, number or phrase and its count on standard output separated by a comma in a new line. For the purpose of this exercise, use space character as the de-limiter for words. String matching should be case sensitive. Here are some examples of output with a sample input file using different arguments:

Contents of Input document (Input.txt): "1000 a big surprise 2000 hello is a big surprise 1000"

PatternCounter Input.txt 1

a, 2
big, 2
surprise, 2
hello, 1
is, 1

---

PatternCounter Input.txt 2

1000, 2
2000, 1

---

PatternCounter Input.txt 3

1000 a big, 1
a big surprise, 2
big surprise 2000, 1
surprise 2000 hello, 1
2000 hello is, 1
hello is a, 1
is a big, 1
big surprise 1000, 1

The output could be empty if there are less than 3 words in the document.

As there are a lot of classes, please check this link for full code.

Code:

Main Class:

public class PatternCounterRunner {

private static PatternCounterService patternCounterService;

public static void main(String[] args) {

    /*inject Keyboard and console to IO runner*/
    IORunner io = new IORunner(new KeyBoardInput(), new ConsoleOutput());
    String inputs[] = io.getUserInputs();
    patternCounterService = new PatternCounterServiceImpl();
    Map<String, Integer> result = null;

    /*Service call to find and return the distinct pattern*/
    try {
        result = patternCounterService.findPatternFromFile(inputs[0], inputs[1]);
    } catch (Exception e) {
        System.err.println("Run time exception occured :" + e);
    }

    io.printResult(result);

}

}

service class:

public class PatternCounterServiceImpl implements PatternCounterService {
PatternFactory patternFactory = new PatternFactory();
IPatternCounter patternCounter;

@Override
public Map<String, Integer> findPatternFromFile(String file, String patternType) throws Exception {
    Map<String, Integer> output = null;
    if (file == null || patternType == null) {
        return output;
    } else {
        patternCounter = patternFactory.getPatternCounter(patternType);
    }
    String[] values = PatternCounterUtils.getContentsFromFile(file);
    if (patternCounter != null) {
        output = patternCounter.getCount(values[0], values[1]);
    } else {
        throw new Exception("Pattern type entered is null or not a number");
    }

    return output;
}

}

this is my factory method to get appropriate counter:

public class PatternFactory {

/**
 * Return an instance of {@linkplain IPatternCounter}
 * @param type - type of pattern counter
 * @return {@linkplain IPatternCounter} instance
 * <br>
 * Presently supported pattern counters with type are:
 * <br>
 * type = 1 => {@link WordCounter}<br>
 * type = 2 => {@link NumberCounter}<br>
 * type = 3 => {@link PhraseCounter}<br>
 * 
 */
public IPatternCounter getPatternCounter(String type) {
    IPatternCounter patternCounter = null;
    if ((type == null) && PatternCounterUtils.isNumeric(type)) {
        return patternCounter;
    } else {
        type = type.trim();
        if (type.equals("1")) {
            patternCounter = new WordCounter();
        } else if (type.equals("2")) {
            patternCounter = new NumberCounter();
        } else if (type.equals("3")) {
            patternCounter = new PhraseCounter();
        }
    }
    return patternCounter;
}
}

Answer 1

First of all I really like how you decomposed your project into a small classes. It's nice and straightforward.

However there are some problems and possible improvements.

General

Print a log message when error occurs (expected value is null), instead of silently doing nothing. You want to know, when something broke.

Information hiding

Classes should protect their data against corruption and outside influence. Make properties of your classes with lowest possible visibility (private). This applies in general, not just properties.

Fail Fast

Your code is very defensive. Most of you methods check for null parameters. Instead consider fail fast rule. Check for nulls at the application boundary, where data enter your application (user input, data read from file, etc...), and throw exception when they don't meet your requirements. In other words. Keep nulls outside. Dont let them enter your application.

Composite names smell

When you are using a composite name such as "patternType" it often indicates a missing abstraction. Since patternType is a fixed set of values consider introducing a PatternType enum. This contributes to the fail fast approach. You either have a valid pattern type or you don't. This also increases type safety of your application. You no longer accept a String but PatternType. This enum could look something like this.

public enum PatternType {
    WORD("1"),
    NUMBER("2"),
    PHRASE("3");

    private final String value;

    PatternType(final String value) {
        this.value = value;
    }

    public static PatternType of(final String type) {
        for (final PatternType pattern : values()) {
            if (Objects.equals(pattern.value, type)) {
                return pattern;
            }
        }
        throw new IllegalArgumentException();
    }
}

KeyBoardInput

• InputStream for the scanner could be provided via constructor instead of hardcoded System.in. This would allow you to easily test it.
• Null check for scanner is redundant as it is never null. It is set a few lines before this check
• You could leverage try with resources to handle scanner so you don't have to care about closing it.

It could look something like this

public class KeyBoardInput implements IStandardInput {

    private final InputStream input;

    public KeyBoardInput(final InputStream input) {
        if (input == null) {
            throw new IllegalArgumentException("Input cannot be null.");
        }
        this.input = input;
    }

    @Override
    public String[] getUserInputs() {
        final String[] inputs = new String[2];
        try (final Scanner scanner = new Scanner(input)) {
            for (int i = 0; i < 2; i++) {
                inputs[i] = scanner.nextLine();
            }
        }
        return inputs;
    }
}

IORunner

There are unnused constructors. Remove them. There is no sence in allowing a construction of invalid object. Again fail fast. Throw IllegalArgumentException when you got invalid StandardInpur or StandardOutput. Now that you got rid of nulls you can remove null checks from IORunner's methods

PatternFactory

Guard check of getPatternCounter will throw NullPointerException when type argument is null

*Counter classes

You should try to split getCount methods into multiple methods. They are deeply nested and very hard to read. It's quite possible that after some cleanup a pattern will emerge that will allow you to share duplicate logic, as logic of these methods is very similar.

Biased Claptrap

Don't name your interfaces ISomething. It is a monstrosity that adds nothing but noice to your code. Interface defines a contract you don't need to know and neither care that it is an interface.

Answer 2

I realize this is old, but I happened to look at it today. And I would reorder your findPatternFromFile method.

public Map<String, Integer> findPatternFromFile(String file, String patternType) throws Exception {
    if (file == null || patternType == null) {
        return null;
    }

    IPatternCounter patternCounter = patternFactory.getPatternCounter(patternType);
    if (patternCounter == null) {
        throw new Exception("Pattern type entered is null or not a number");
    }

    String[] values = PatternCounterUtils.getContentsFromFile(file);
    return patternCounter.getCount(values[0], values[1]);
}

First, this checks for null. If those checks are necessary, then there is no reason not to do them right at the beginning. They do not depend on the rest of the code in any way. So do them first. It's not clear to me why this returns rather than throwing an exception, but it's not clear to me that that's wrong either. For now, I'll assume that you've considered the alternatives and chosen correctly.

I don't see the point of using an object property for the patternCounter. You fetch it even if that is already initialized. So why not use a local variable instead?

Next, check if there is a patternCounter for the patternType. This is another possible failure point, so you might as well check it next. The only advantage of doing the getContentsFromFile next is that it might fail and save you the work. But I would tend to expect a file access to be more work than mapping one variable to another. So I would tend to do the mapping first. Then if there's a problem, it's unnecessary to access the file at all. Your original had the worst of both worlds, doing the mapping first and then getting the contents of the file even if the mapping failed.

The exception message is misleading. At that point, we know that the pattern type is not null. And I'm not sure that we know that it is not a number. It could be (for example) 4 or 33. Those would also fail, even though they are numbers. If you move that exception into getPatternCounter, you could potentially produce a more precise and accurate message.

I'm not crazy about having getContentsFromFile return an array of two String. I'm not sure how I'd change it though, as that method's one of the things that you elided. I have no idea what is in that array. I would have expected a simple String representing the file contents.

There's no point in declaring an output variable. You only use it to hold null or the result of getCount. There is no additional logic nor manipulation of the returned result. It's just as easy to return null and the result of getCount directly. I originally rewrote the null check on patternCounter to allow output to be declared and initialized at the same time. It was only after that, that I realized that output wasn't necessary at all.

Overall, this version is shorter and in my opinion, it flows better. It checks the input arguments and fails as soon it finds a problem. So we don't waste time processing after we know that it's broken. There's only three lines actively doing things. The other four are just checking the arguments.