12
\$\begingroup\$

I wrote a class that can convert to and fro from Roman Numerals and Decimals. I would appreciate critical points on everything, especially:

  1. Shortening my code.
  2. Choosing a better approach.
  3. Using better/new features of Java 8.

And so on...

I didn't add JavaDoc because I am expecting suggestions for a better approach. I will document it then. Here is the class:

RomanNumeral.java

package library.util;

import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.Map;

/**
 * A "recreational" class that provides that functionality of converting
 * values to and fro from Roman numerals.
 *
 * @author Subhomoy Haldar
 */
public class RomanNumeral {

    private final String representation;
    private final int value;

    private RomanNumeral(String representation, int value) {
        this.representation = representation;
        this.value = value;
    }

    @Override
    public String toString() {
        return representation;
    }

    public int getValue() {
        return value;
    }

    public static RomanNumeral of(String representation) throws
            NumberFormatException,
            ArithmeticException {
        // Zero wasn't supported then...
        if (representation.isEmpty()) {
            throw new NumberFormatException("Empty String.");
        }
        int value = 0, previous = 0;
        StringBuilder builder = new StringBuilder(representation);
        builder.reverse();  // go in the opposite manner
        for (int i = 0; i < builder.length(); i++) {
            try {
                int current = map.get(String.valueOf(builder.charAt(i)));
                // If the value suddenly drops then subtract, don't add.
                value += current < previous ? -current : current;
                previous = current;
            } catch (NullPointerException e) {
                // A NullPointerException will be thrown only if the
                // character is not present in the map.
                throw new NumberFormatException
                        ("Unrecognised character : " + builder.charAt(i));
            }
        }
        // calling representationOf() makes sure that "IIIII" becomes "V".
        return new RomanNumeral(representationOf(value), value);
    }

    public static RomanNumeral valued(int value) {
        return new RomanNumeral(representationOf(value), value);
    }

    public static String representationOf(int value)
            throws ArithmeticException {
        // The valid range is (0, 5000)
        if (value <= 0 || value >= 5_000) {
            throw new ArithmeticException("Unsupported value : " + value);
        }
        int copy = value;
        StringBuilder builder = new StringBuilder(10);  // 4999 gives 10 chars
        // The descending order is maintained
        for (Map.Entry<String, Integer> entry : map.entrySet()) {
            int v = entry.getValue();
            String k = entry.getKey();
            while (copy >= v) {
                copy -= v;
                builder.append(k);
            }
        }
        return builder.toString();
    }

    private static final Map<String, Integer> map;

    static {
        // Using LikedHashMap ensures retention of order.
        Map<String, Integer> temporary = new LinkedHashMap<>(13);
        // Initialize in descending order.
        temporary.put("M", 1_000);
        temporary.put("CM", 900);
        temporary.put("D", 500);
        temporary.put("CD", 400);
        temporary.put("C", 100);
        temporary.put("XC", 90);
        temporary.put("L", 50);
        temporary.put("XL", 40);
        temporary.put("X", 10);
        temporary.put("IX", 9);
        temporary.put("V", 5);
        temporary.put("IV", 4);
        temporary.put("I", 1);
        // Make an unmodifiable map out of it .
        map = Collections.unmodifiableMap(temporary);
    }

}

The algorithm for converting from Roman to Decimal is adapted from this post.

\$\endgroup\$
5
\$\begingroup\$

I like that you use an immutable map. However I don't like the name map: you should give it a more descriptive name and it should be capitalized since it is a constant. You should also declare the type of map as LinkedHashMap: it is important here since you do use the ordering property. You can define map as private static final Map<String, Integer> map = new LinkedHashMap<String, Integer>() {{ this.put(x, y); ... }}.

Your api should probably return an error on invalid roman numeral instead of: // calling representationOf() makes sure that "IIIII" becomes "V".

In method of (string to int), I think you don't have to use a StringBuilder and to invert it. You can easily modify your algorithm to work forward and just use toCharArray. You could use the Java 8 "functional" (Stream) features here instead of a for-loop. I like functional programming since it makes for shorter and more readable code, but they really messed things up in Java 8 and I couldn't find an easy solution. I could do it in Scala, but in Java 8 it would end up being longer than your code and completely unreadable.

\$\endgroup\$
  • \$\begingroup\$ I like your answer, especially the part about the naming and the types. But I would appreciate if you provided some code to substantiate your claim about the representation method. \$\endgroup\$ – Hungry Blue Dev Sep 21 '15 at 11:32
  • 1
    \$\begingroup\$ The problem with a functional approach is, that dismantling the number into it's parts is inherently mutating the value you work on. As such a functional solution is bound to be clunky. \$\endgroup\$ – Vogel612 Sep 21 '15 at 12:12

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.