"Merchants Guide to Galaxy" challenge

Question

I have solved the classic "Merchants Guide to Galaxy" problem:

A merchant buys and sells items in the galaxy. Buying and selling over the galaxy requires you to convert numbers and units. The numbers used for intergalactic transactions follows similar convention to the roman numerals. Roman numerals are based on seven symbols:

I 1
V 5
X 10
L 50
C 100
D 500
m 1000

Numbers are formed by combining symbols together and adding the values. For example, MMVI is 1000 + 1000 + 5 + 1 = 2006. Generally, symbols are placed in order of value, starting with the largest values. When smaller values precede larger values, the smaller values are subtracted from the larger values, and the result is added to the total. For example MCMXLIV = 1000 + (1000 − 100) + (50 − 10) + (5 − 1) = 1944.

The symbols "I", "X", "C", and "M" can be repeated three times in succession, but no more. (They may appear four times if the third and fourth are separated by a smaller value, such as XXXIX.) "D", "L", and "V" can never be repeated. "I" can be subtracted from "V" and "X" only. "X" can be subtracted from "L" and "C" only. "C" can be subtracted from "D" and "M" only. "V", "L", and "D" can never be subtracted. Only one small-value symbol may be subtracted from any large-value symbol. A number written in [16]Arabic numerals can be broken into digits. For example, 1903 is composed of 1, 9, 0, and 3. To write the Roman numeral, each of the non-zero digits should be treated separately. Inthe above example, 1,000 = M, 900 = CM, and 3 = III. Therefore, 1903 = MCMIII. Input to your program consists of lines of text detailing your notes on the conversion between intergalactic units and roman numerals. You are expected to handle invalid queries appropriately.

Test input

glob is I
prok is V
pish is X
tegj is L
glob glob Silver is 34 Credits
glob prok Gold is 57800 Credits
pish pish Iron is 3910 Credits
how much is pish tegj glob glob ?
how many Credits is glob prok Silver ?
how many Credits is glob prok Gold ?
how many Credits is glob prok Iron ?
how much wood could a woodchuck chuck if a woodchuck could chuck wood ?

Test Output

pish tegj glob glob is 42
glob prok Silver is 68 Credits
glob prok Gold is 57800 Credits
glob prok Iron is 782 Credits
I have no idea what you are talking about

Could someone please look into it from the perspective of design patterns and good coding practices and could possibly give me a constructive feedback?

The git link for the entire project is here.. I would recommend to please go through the git link, as it would make much more sense.

BeginMerchantsGuide.java

public class BeginMerchantsGuide 
{
    public static void main( String[] args )
    {
        String filePath = "input.txt";

        if (args.length != 0)
            filePath = args[0];

        try{
            ReadInputAndProcess readInput = new ReadInputAndProcess();
            readInput.readFileAndProcess(filePath);
            DisplayOutput.processOutput();
        }
        catch(Exception e){
            System.out.println("An exception has been raised, Either due to invalid input or due to reading the file, quiting the program " + e);
        }

    }
}

ReadInputAndProcess.java

    /**
 * Class to read and process the data at the same time
 * The processing starts as soon as the input is received
 *
 */
public class ReadInputAndProcess {

    public static final String NO_IDEA = "I have no idea what you are talking about";

    private static Map<String, Roman> interGalacticRomanMapping = new HashMap<>();
    private static Map<String, Double> objectSoldPerUnitValue = new HashMap<>();

    /**
     * reads and process the input file
     * @param fileName
     */
    public void readFileAndProcess(String fileName) throws Exception{

        try(BufferedReader br = new BufferedReader(new FileReader(fileName))) {

            String line;

            while((line = br.readLine()) != null)
            {
                processInput(line.trim());
            }

        } catch (IOException e) {
            System.out.println("File not found exception " + e);
        } 
    }

    /**
     * Parses the input line by line and decides the type of request and appropriately forwards the request
     * @param line
     * @throws Exception
     */
    private void processInput(String line) throws Exception{
        //split by whitespace
        line = line.toLowerCase();
        String[] inputs = line.split("\\s+");
        List<String> inputQuestions = DisplayOutput.inputQuestions;
        List<String> outputValues = DisplayOutput.outputValues;

        if(line.startsWith("how many credits is")){
            inputQuestions.add(line);
            outputValues.add(String.valueOf(generateCreditValue(Arrays.copyOfRange(inputs, 4, inputs.length-1))));
        }
        else if(line.startsWith("how much is")){
            inputQuestions.add(line);
            outputValues.add(String.valueOf(generateGalacticUnitToNumericValue(Arrays.copyOfRange(inputs, 3, inputs.length-1))));
        }
        else if(line.contains("is") && !line.contains("credits"))
            mapInterGalacticToRomanUnits(inputs);
        else if(line.contains("is") && line.contains("credits"))
            generateObjectSoldPerUnitMap(inputs);
        else{
            inputQuestions.add(line);
            outputValues.add(NO_IDEA);
        }

    }

    /**
     * Receives inputs of the form "glob is I"
     * maps the alien language to the RomanType
     * @param arr
     */
    private void mapInterGalacticToRomanUnits(String[] arr){
        try{
            interGalacticRomanMapping.put(arr[0], Roman.valueOf(arr[2]));
        }
        catch(IllegalArgumentException e){
            System.out.println("This type of Roman is not defined, exiting the program " + e);
        }
    }

    /**
     * Receives input of the form "glob glob Silver is 34 Credits"
     * Creates a map of the object sold and (value/unit)
     * returns -1 in case of a validation error
     * @param arr
     * @return
     * @throws Exception
     */
    private int generateObjectSoldPerUnitMap(String[] arr) throws Exception{
        StringBuilder romanNumeral = new StringBuilder();
        int i;
        for(i = 0; i<arr.length; i++){
            Roman roman = interGalacticRomanMapping.get(arr[i]); 
            if(roman != null){
                romanNumeral.append(roman.getDisplayValue());
            }
            else
                break;
        }

        int value = RomanValidator.validateRoman(romanNumeral.toString());
        if(value == -1)
            return -1;
        String objectName = arr[i];
        int credit = Integer.parseInt(arr[i + 2]);

        objectSoldPerUnitValue.put(objectName, (double)credit/value);
        return 1;
    }

    /**
     * Receives input of the form "pish tegj glob glob" for questions like "how much is pish tegj glob glob ?"
     * returns -1 in case of validation exception
     * @param arr
     * @return
     * @throws Exception
     */
    private int generateGalacticUnitToNumericValue(String[] arr){

        try {
        String romanNumeral = generateRomanFromGalacticUnit(arr);

        if(romanNumeral == null)
            return -1;

            return  RomanValidator.validateRoman(romanNumeral.toString());
        } catch (Exception e) {
            return -1;
        }

    }


    /**
     * Receives input of the form "glob prok Silver" for questions like "how many Credits is glob prok Silver ?"
     * returns -1 in case of validation exception
     * @param arr
     * @return
     * @throws Exception
     */
    private Double generateCreditValue(String[] arr){
        int currentValue = generateGalacticUnitToNumericValue(Arrays.copyOfRange(arr, 0, arr.length-1));

        if(currentValue == -1)
            return (double)-1;

        return currentValue * objectSoldPerUnitValue.get(arr[arr.length-1]);
    }

    /**
     * a utils method to generate Roman string from Alien input 
     * @param arr
     * @return
     */
    private String generateRomanFromGalacticUnit(String[] arr){
        StringBuilder romanNumeral = new StringBuilder();
        int i;
        for(i = 0; i< arr.length; i++){
            Roman roman = interGalacticRomanMapping.get(arr[i]);
            if(roman != null){
                romanNumeral.append(roman.getDisplayValue());
            }
            else
                break;
        }

        if(i != arr.length)
            return null;

        return romanNumeral.toString();
    }

}

RomanNumbers.java

    public class RomanNumbers {

    public enum Roman{
        i("i", 1), v("v", 5), x("x", 10), l("l", 50), c("c", 100), d("d", 500), m("m", 1000);

        private int value;
        private String displayValue;
        Roman(String displayValue, int value){
            this.displayValue = displayValue;
            this.value = value;
        }

        public int getValue() {
            return value;
        }

        public String getDisplayValue() {
            return displayValue;
        }


    }
}

RomanValidator.java

    public class RomanValidator {

    private RomanValidator(){

    }

    private static final Set<Character> THREE_TIMES_REPEATED_CHARACTERS = new HashSet<Character>();
    private static final Set<Character> NOT_SUBTRACTED_CHARACTERS = new HashSet<Character>();

    static{
        THREE_TIMES_REPEATED_CHARACTERS.add('i');
        THREE_TIMES_REPEATED_CHARACTERS.add('x');
        THREE_TIMES_REPEATED_CHARACTERS.add('c');
        THREE_TIMES_REPEATED_CHARACTERS.add('m');

        NOT_SUBTRACTED_CHARACTERS.add('v');
        NOT_SUBTRACTED_CHARACTERS.add('l');
        NOT_SUBTRACTED_CHARACTERS.add('d');
        NOT_SUBTRACTED_CHARACTERS.add('m');
    }

    /**
     * Validate the input RomanNumber and return it's integer value if valid, else return -1
     */
    public static int validateRoman(String romanNumber) throws Exception{
        char[] charArray = romanNumber.toCharArray();
        char previousChar = ' ';

        int characterRepeatCount = 1;
        int total = 0;
        int previousCharacterOrdinal = Integer.MAX_VALUE; 
        int currentCharacterOrdinal;

        for(int i = 0; i<charArray.length; i++){
            char currentChar = charArray[i];
            int currentRomanCharNumericValue = Roman.valueOf(String.valueOf(currentChar)).getValue();

            if(previousChar != ' '){
                previousCharacterOrdinal = Roman.valueOf(String.valueOf(previousChar)).ordinal();
            }
            currentCharacterOrdinal = Roman.valueOf(String.valueOf(currentChar)).ordinal();

            if(currentChar == previousChar && ++characterRepeatCount < 4 && THREE_TIMES_REPEATED_CHARACTERS.contains(currentChar)){
                total += currentRomanCharNumericValue;
            }
            else if(characterRepeatCount > 3){
                total = -1;
            }
            else if(currentChar == previousChar && !THREE_TIMES_REPEATED_CHARACTERS.contains(currentChar)){
                total = -1;
            }
            else if(previousCharacterOrdinal < currentCharacterOrdinal  && !NOT_SUBTRACTED_CHARACTERS.contains(previousChar)){
                int previousRomanCharNumericValue = Roman.valueOf(String.valueOf(previousChar)).getValue();
                if(previousCharacterOrdinal + 2 >= currentCharacterOrdinal){
                    total += currentRomanCharNumericValue - (2 * previousRomanCharNumericValue); 
                    characterRepeatCount = 1;
                }
                else{
                    total = -1;
                }
            }
            else if(previousCharacterOrdinal < currentCharacterOrdinal  && NOT_SUBTRACTED_CHARACTERS.contains(previousChar)){
                total = -1;
            }
            else{
                characterRepeatCount = 1;
                total += currentRomanCharNumericValue;
            }
            previousChar = currentChar;
            if(total == -1)
                break;
        }
        return total;
    }

}

DisplayOutput.java

    public class DisplayOutput {

    private DisplayOutput(){

    }

    //these two lists hold question asked and out for same indexes
    //this will hold all the questions asked in the input  
    public static final List<String> inputQuestions = new ArrayList<>();
    //this holds the output of the questions and special values if the question was not valid or a validation failure occurs  
    public static final List<String> outputValues = new ArrayList<String>();

    //String constants
    public static final String HOW_MUCH_IS = "how much is";
    public static final String HOW_MANY_CREDITS_IS = "how many credits is";
    public static final String IS = " is ";
    public static final String CREDITS = " credits";

    /**
     * Display the output based on the lists
     */
    public static void processOutput(){

        for (int i = 0; i < inputQuestions.size(); i++) {
            StringBuilder result = new StringBuilder();

            String question = inputQuestions.get(i);
            String output = outputValues.get(i);

            if(output == ReadInputAndProcess.NO_IDEA || Double.valueOf(output).intValue() == -1){
                result.append(ReadInputAndProcess.NO_IDEA);
            }
            else{
                if(question.startsWith(HOW_MUCH_IS)){
                    result.append(question.substring(HOW_MUCH_IS.length(), question.length()-1).trim());
                    result.append(IS);
                    result.append(Double.valueOf(output).intValue());
                }

                else if(question.startsWith(HOW_MANY_CREDITS_IS)){
                    result.append(question.substring(HOW_MANY_CREDITS_IS.length(), question.length()-1).trim());
                    result.append(IS);
                    result.append(Double.valueOf(output).intValue());
                    result.append(CREDITS);
                }
                else
                    result.append(ReadInputAndProcess.NO_IDEA);
            }

            System.out.println(result.toString());
        }
    }

}

Answer 1

Preface: I did not address everything that feels off. I don't think I have the skill to do so. But I hope that this already helps.

The biggest design problem that I see is that the ReadInputAndProcess class and the DisplayOutput class communicate only with the help of two public lists in DisplayOutput. This hides the communication between your classes in a way you don't want. In this case I would make the lists parameters of the processOutput() method. The readFileAndProcess() method would then return them. This way you can also easier understand the main method. When I read it at first I had no idea about the communication between those two classes.

It is good that you use constants in DisplayOutput for the text you print, but why are you not using constants for the text to compare to in ReadInputAndProcess?

Some smaller things:

When an error happens, you print an error message, and e. If you would also do e.printStackTrace(), you would get way more information.

While I know that you don't need a block for an if-statement, it makes the code harder to read. It also makes it harder to maintain. If I want to add another line there, I need to be careful to also add a block, otherwise it will behave weird.

You use Double.valueOf(intInAString).intValue() to parse an int. Integer.parseInt(intInAString) is more readable and will also fail on "12.5", as it should.

Your methods should never just throw Exception. This gives no information about where those might be thrown, and why. Throw more specific exceptions, and write a comment about when they might be thrown.

Answer 2

enum Roman

• Should be called RomanDigit.
• It's in the otherwise pointless class RomanNumbers. Either let Roman(Digit) be stand alone , or collect all functionality to do with roman numbers in it that class.
• The name of the fields and the "display values" are identical. I'd either drop the display value, or at least use it to identify the digits instead of using Roman.valueOf() which uses the field names.
• It is basically a glorified map of characters to values. It would make sense to store all information about the digits in it, such as if they can be repeated or subtracted from a larger digit, instead of "hiding" that information in the "validator".
• The information which digits can be subtracted from which other digits is "hidden" inside the validation algorithm. This too should be provided by Roman(Digit).

Generally: You are using this enum halfheartedly, so that you are juggling the enum, chars and ordinals in the validation algorithm. Either move all information into the enum as I suggest above, or drop it all together.

---

RomanValidator.validateRoman

• First off this isn't a "validator" but a converter.
• It should be "grouped" together with Roman(Digit) in a package or a class.
• There is no need to convert the string to a char array.
• It handles errors/problems very poorly. As Ayane said, throwing (but also catching) generic Exceptions is a bad idea, and having the method just return -1 in case of an error isn't good either, because if the "user" doesn't check for this, they will simply continue working with an otherwise fine number instead of getting an obvious error. At the very least use Integer instead of int and return null. Better would be to throw specific exceptions which describe the problem.
• You repeatedly use Roman.valueOf(String.valueOf(...)) intstead of storing the result.
• It's buggy. It returns 10 for iix instead of -1.

Here's my suggestion how I would do it: http://ideone.com/AyEuL0

public enum RomanDigit {
    i(1, true), v(5, false), x(10, true), l(50, false), c(100, true), d(500, false), m(1000, true);

    private int value;
    private boolean repeatable;

    RomanDigit (int value, boolean repeatable){
        this.value = value;
        this.repeatable = repeatable;
    }

    public int getValue() {
        return value;
    }

    public boolean isRepeatable() {
        return repeatable;
    }

    public boolean subtractableFrom(RomanDigit other) {
        // Currently subtractale digits happen to be the same ones that are repeatable.
        // Should this change, then another solution is needed.
        if (other == null || !this.isRepeatable()) {
            return false;
        }

        int oridinal = this.ordinal();
        int otherOridinal = other.ordinal();

        return (oridinal == otherOridinal - 1 ||  oridinal == otherOridinal - 2);
    }
}

public static int convertRomanNumber(String romanNumber) {
    RomanDigit previousDigit = null;
    int characterRepeatCount = 1;
    int total = 0;

    for(int i = 0; i < romanNumber.length(); i++){
        // Also throws IllegalArgumentException if a digit is invalid
        RomanDigit currentDigit = RomanDigit.valueOf(String.valueOf(romanNumber.charAt(i)));
        int currentRomanCharNumericValue = currentDigit.getValue();

        if (currentDigit.equals(previousDigit)) {
            characterRepeatCount++;

            if (characterRepeatCount > 3) {
                throw new IllegalArgumentException("Repeatable Digit is repeated too often"); // Error message needs more details
            }
            if (currentDigit.isRepeatable()) {
                total += currentRomanCharNumericValue;
            } else {
                throw new IllegalArgumentException("Unrepeatable Digit is repeated"); // Error message needs more details
            }
        } else if (previousDigit != null && previousDigit.compareTo(currentDigit) < 0) {
            if (characterRepeatCount > 1) {
                throw new IllegalArgumentException("Repeatable Digit is repeated before larger digit"); // Error message needs more details
            }
            if (previousDigit.subtractableFrom(currentDigit)) {
                characterRepeatCount = 1;
                total += currentRomanCharNumericValue - (2 * previousDigit.getValue()); 
            } else {
                throw new IllegalArgumentException("Digit may not be subtracted from other digit"); // Error message needs more details
            }
        } else {
            characterRepeatCount = 1;
            total += currentRomanCharNumericValue;
        }

        previousDigit = currentDigit;
    }
    return total;
}

---

UnFortunatly I don't have more time right now for the rest.