5
\$\begingroup\$

I am learning Java and this is my solution for a program that will read two numbers in the range of 0 to 99 and multiply them. Input numbers are given in natural language e.g. for input thirty-two ten it should return 320. I have tried to do my best. Could someone please review this and suggest improvements, if any?

package assignments;
import java.util.*;
public class NaturalLanguageMultiply {
    private static final Map<String, Integer> numericValues = new HashMap<String, Integer>();
    private static final String WORD_SEPARATOR = "-";
    static {
        numericValues.put("zero", 0);
        numericValues.put("one", 1);
        numericValues.put("two", 2);
        numericValues.put("three", 3);
        numericValues.put("four", 4);
        numericValues.put("five", 5);
        numericValues.put("six", 6);
        numericValues.put("seven", 7);
        numericValues.put("eight", 8);
        numericValues.put("nine", 9);
        numericValues.put("ten", 10);
        numericValues.put("eleven", 11);
        numericValues.put("tweleve", 12);
        numericValues.put("thirteen", 13);
        numericValues.put("fourteen", 14);
        numericValues.put("fifteen", 15);
        numericValues.put("sixteen", 16);
        numericValues.put("seventeen", 17);
        numericValues.put("eighteen", 18);
        numericValues.put("nineteen", 19);
        numericValues.put("twenty", 20);
        numericValues.put("thirty", 30);
        numericValues.put("forty", 40);
        numericValues.put("fifty", 50);
        numericValues.put("sixty", 60);
        numericValues.put("seventy", 70);
        numericValues.put("eighty", 80);
        numericValues.put("ninety", 90);
    }

    private static int wordToNumber(String word) {
        String[] tokens = word.split(WORD_SEPARATOR);
        int number = 0;
        for(String token : tokens) {
            Integer numericValue = numericValues.get(token);
            if(numericValue == null) {
                throw new IllegalArgumentException("unknown token " + token + " in word " + word);
            }

            number += numericValue;
        }
        return number;
    }


    public static void main(String[] args) {

        if(args.length < 2) {
            System.out.println("argument missing");
            System.exit(1);
        }

        try {
            int firstNumber = wordToNumber(args[0]);
            int secondNumber = wordToNumber(args[1]);
            System.out.format("%d%n", firstNumber * secondNumber);
            System.exit(0); 
        } catch(Exception e) {
            System.out.println(e.getMessage());
            System.exit(1);
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ you have written "fourty" instead of "forty" \$\endgroup\$ – MrBrushy Feb 10 '17 at 13:27
4
\$\begingroup\$

General

I do not know exactly why your question remains unanswered till now. But I guess "exercises" tend to have less attention as this exercise seems to be really basic. Do not get me wrong. Everybody had to go through basic things.

So the point is that you found a solution to a problem. And so far you did very well. But I want encourage you to keep on going as this is only the surface. Here are the things every programmer has to go through.

  1. Learn the theory of sequence, selection and iteration

  2. Train your algorithmic thinking by solving problems with the elements of 1. in a programming language of your choice by increasing difficulty

  3. Get familiar with the language mechanisms

  4. Apply programming paradigms like functional or object-oriented programming

  5. Formalize you code fragments by learning the currently identified 26 design patterns and learn to to apply them in the correct situations

  6. Organize your code by following the correct semantic of each code fragment and learn and apply the SOLID principles as they guide you through the jungle of design decisions

  7. Learn the restrictions of the language you use for a problem. Maybe in another language you are able to express the solution in another way that is more elegant.

The point is that the code provided can only be judged under point 1, 2 and 3. And there your are on a good way. As I do not know how deep you dived into the java programming language maybe it is time to apply the object-oriented or functional programming paradigm.

As your code currently is procedural I suggest to formulate your code in a more object-oriented way.

Suggestions

Reduce static elements

The static modifier is not overall bad (e.g. constants) but for methods it hinders you to benefit from the OO mechanism "polymorphism".

Introduce OO

Try to formulate your code with objects. To start here remove all static modifiers from all methods but the the main-method. Introduce a constructor for your class "NaturalLanguageMultiply". Pass in the arg-Array make the validation and store it in a local variable.

Do not exit with System.exit(0);

This does not make sense at all as every program will leave with the exit code 0 if the program ends. It is a needless statement.

Exception handling

I suggest to have a single concept of exception handling. If you want to promote execution failure of a program by calling System.exit(...) then do this in the final statement in your program. Internally you should rely on exceptions. So your argument validation should not exit the JVM but throwing an IllegalArgumentException.

Currently you catch "Exception". I suggest to catch expected Exceptions explicitly. This helps you to return distinguishable exit codes.

Furthermore avoid System.out.println at arbitrary locations in your code to promote exceptions. This will cause redundant code spread all over the place. Promote the exceptions where you are going to handle them.

Provide a calculation-method

Introduce a method called getResult() to do the multiplication.

Naming

As mentined in the comments naming things properly will help others to read your code. I would change the constant name from "WORD_SEPARATOR" to "TOKEN_SEPARATOR" and renaming "args" into "words". This how it makes sense to me. You have two words that are both spoken numbers. And one word is partitioned into token by a separator of those tokens. Maybe you can also rename "words" into "spokenNumbers" and "word" into "spokenNumber". But I think I made case.

Code

public class NaturalLanguageMultiply {


    private static final Map<String, Integer> numericValues = new HashMap<String, Integer>();


    private static final String TOKEN_SEPARATOR = "-";


    static {
        numericValues.put("zero", 0);
        numericValues.put("one", 1);
        numericValues.put("two", 2);
        ...
        numericValues.put("ninety", 90);
    }



    private String[] words;


    NaturalLanguageMultiply(String[] words) {

        if(words.length < 2) {
            throw new IllegalArgumentException("argument missing");
        }

        this.words = words;

    }


    private int wordToNumber(String word) {

        String[] tokens = word.split(TOKEN_SEPARATOR);

        int number = 0;

        for (String token: tokens) {

            Integer numericValue = numericValues.get(token);

            if(numericValue == null) {
                throw new IllegalArgumentException("unknown token " + token + " in word " + word);
            }

            number += numericValue;
        }

        return number;
    }


    private int getResult() {

        int firstNumber = wordToNumber(words[0]);
        int secondNumber = wordToNumber(words[1]);

        return firstNumber * secondNumber;
    }


    public void main(String[] args) {

        try {

            int result = new NaturalLanguageMultiply(args).getResult();

            System.out.format("%d%n", result);

        } catch (IllegalArgumentException e) {

            System.out.println(e.getMessage());
            System.exit(1);

        } catch (Exception e) {

            System.out.println(e.getMessage());
            System.exit(2);

        }

    }


}

Further steps

Your solution parses and interpretes incoming data and processes calculations. This is best addressed with the state pattern and the interpreter pattern. This is an advanced technique you should have in mind when you feel ready for it.

\$\endgroup\$
  • 1
    \$\begingroup\$ Strange use of 'word' and 'token'. If a word is an input argument, then it's propably a 'sentence' that is split into 'words', don't you think? ('token' is fine for generic matching, but here it's not very natural) \$\endgroup\$ – MrBrushy Feb 10 '17 at 13:35
2
\$\begingroup\$

Input validation

Your current method accepts a string like twenty-twelve to represent 32, which may not be desirable. You may want to validate whether the first or second token can be combined with another, e.g. rejecting twenty-twelve because twelve cannot be a second token, and accepting thirty-two.

For example, you can have a NumberToken class:

public class NumberToken {
    private final int value;
    private final String text;
    private final Position position;

    public NumberToken(int value, String text, Position position) {
        // constructor
    }

    // getters...
}

public enum Position {
    FIRST, SECOND, BOTH;
}

The enumerated values of Position allow us to validate how a token can be combined with other tokens. For example:

public NumberToken two = new NumberToken(2, "two", Position.BOTH);
public NumberToken twelve = new NumberToken(12, "twelve", Position.FIRST);
public NumberToken twenty = new NumberToken(20, "twenty", Position.FIRST);
public NumberToken thirty = new NumberToken(30, "thirty", Position.FIRST);

For starters, the validation that can be done are:

  1. Position.FIRST and Position.SECOND must be in the respective placements.
  2. Position.BOTH cannot be combined with another Position.BOTH.

You should also think about edge cases, like how to reject twelve-two (e.g. a new value like Position.FIRST_ONLY for twelve?).

One nice thing about this approach is that it's quite easy to generate the lookup Map using the new stream-based processing:

private static final Map<String, NumberToken> LOOKUP = 
    Arrays.stream(one, two, /* ... */ ninety)
            .collect(Collectors.toMap(NumberToken::getText, Function.identity()));

Error output

Error output should go to System.err instead of System.out:

try {
    // ...
    // Below System.exit(int) is unnecessary, status will be 0 by default
    // System.exit(0); 
} catch(Exception e) {
    System.err.println(e.getMessage());
    System.exit(1);
}

Generic type inference

Since Java 7, you can use <> to represent the desired generic type at variable declaration as such:

private static final Map<String, Integer> numericValues = new HashMap<>();
\$\endgroup\$

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.