3
\$\begingroup\$

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;
}
}
\$\endgroup\$
  • 2
    \$\begingroup\$ @Arun Sam's comment is standard operating procedure for this site. Note that while Sam explained the issue, it was a moderator who closed your post. You won't get any code reviews unless you can include your code. If you have too much code then post the important parts, although I've seen questions with 4 or 5 classes posted. Especially as long as they aren't too long, it isn't a big deal. It's just how we roll. \$\endgroup\$ – Conor Mancone Nov 9 '17 at 20:31
  • \$\begingroup\$ @ConorMancone I posted the important parts of the code and made sure this wont clog up whole post. :) \$\endgroup\$ – Arun Nov 9 '17 at 20:51
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I deliberately gulp all exceptions because the output must be null or the result. I was displaying minimum exceptions as possible. The review was top notch and I will take care of your comments in the future. Thank again @Januson \$\endgroup\$ – Arun Nov 12 '17 at 0:00

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.