Let's revisit Roman numbers

Problem Statement:


Write a function to convert from normal numbers to Roman Numerals:
e.g.

1  => I
10  => X
 7  => VII

Code:

import java.util.HashMap;
import java.util.Map;
import java.util.NavigableMap;
import java.util.TreeMap;

public class RomanNumeral {
  private final int number;
  private static final Map CACHE = new HashMap<>();

  private static final NavigableMap NUMBER_TO_ROMAN =
      initMap();

  public RomanNumeral(int number) {
    if (number  3000) {
      throw new IllegalArgumentException("Number out of range.");
    }
    this.number = number;
  }

  public String getRomanNumeral() {
    if (number == 0) {
      return "";
    }
    String result = CACHE.get(number);
    if (result != null) {
      return result;
    }
    result = toRomen(number);
    CACHE.put(number, result);
    return result;
    //return number == 0 ? "" : toRomen(number);
  }

  private static String toRomen(int num) {
    int floorNum = NUMBER_TO_ROMAN.floorKey(num);
    // Base case
    if (floorNum == num) {
      return NUMBER_TO_ROMAN.get(num);
    }
    return NUMBER_TO_ROMAN.get(floorNum) + toRomen(num - floorNum);
  }

  private static NavigableMap initMap() {
    NavigableMap map = new TreeMap<>();
    map.put(1, "I");
    map.put(4, "IV");
    map.put(5, "V");
    map.put(9, "IX");
    map.put(10, "X");
    map.put(40, "XL");
    map.put(50, "L");
    map.put(90, "XC");
    map.put(100, "C");
    map.put(400, "CD");
    map.put(500, "D");
    map.put(900, "CM");
    map.put(1000, "M");
    return map;
  }
}

Test Suite:

```
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

import java.util.Arrays;
import java.util.Collection;

import static org.junit.Assert.assertEquals;

@RunWith(Parameterized.class)
public class RomanNumeralsTest {

private int input;
private String expectedOutput;

@Parameters
public static Collection data

Your code is very clear and nicely written. Also, using a TreeMap to take advantage of ordered keys is great insight.

Overall, I practically only have good comments about the code:

• Throwing an IllegalArgumentException in case of a negative or greater than 3000 number is a great way to document the fact those values are not allowed.

• The recursive method toRomen handles all the case correctly. You made a small typo in the name of the method: it should be toRoman instead of toRomen.

• Coding against the NavigableMap interface and hiding the TreeMap concrete instantiation inside a method is a nice thing to do.

• Using a cache might indeed be a good idea: it helps to ensure that the same roman numerals are not converted each time and, since we're only interested in 3000 numbers at maximum, this map won't become gigantic and consume a lot of memory.

I still have a couple of suggestions:

-
You are currently handling the number 0 in a special early return inside getRomanNumeral with:

if (number == 0) {
    return "";
}

This isn't strictly needed. You have done that because toRomen doesn't handle 0 as input. Handling 0 could be done inside toRomen instead. Consider the following:

private static String toRoman(int num) {
    if (num == 0) {
        return "";
    }
    int floorNum = NUMBER_TO_ROMAN.floorKey(num);
    return NUMBER_TO_ROMAN.get(floorNum) + toRoman(num - floorNum);
}

Now the method toRoman has a clear-cut base case: when the number given is 0, we return an empty String. This also takes care of the recursive call with num - floorNum when both are equal: we will fall under this case and return the empty String.

-
With the above change, getRomanNumeral now becomes:

public String getRomanNumeral() {
    String result = CACHE.get(number);
    if (result != null) {
        return result;
    }
    result = toRomen(number);
    CACHE.put(number, result);
    return result;
}

We could use methods introduced in Java 8 to make this shorter. Mainly, we're interested in the computeIfAbsent(key, mappingFunction) method here. This method will return the currently mapped value to the given key, or invoke the given mapping function if there are no mapping. Therefore, we can simply have here:

public String getRomanNumeral() {
    return CACHE.computeIfAbsent(number, RomanNumeral::toRoman);
}

When the cache already contains the value for number, it will be returned directly. Otherwise, the method toRoman will be invoked by giving it the number as parameter, the cache will be updated with the value returned by the function and this value will be returned.

StackExchange Code Review Q#128777, answer score: 5