Java converter from Arabic to Roman Numerals

Question

I am learning Java and would like to get feedback on the code I wrote.

I have written an implementation of an algorithm (found here) for converting any decimal Arabic number to Roman numeral string. My concerns are:

• The code seems too long for some methods.
• It is not clear what is going on in the class (unless you already know what the algorithm is doing).
• Correct use of OOP
• Tight coupling of methods
• Are there Java 8 features that I should be using but not using.

I am looking for your input, advice and feedback so that I can get better at writing clean and maintainable code.

import java.util.HashMap;
import java.util.Map;
import java.util.stream.Stream;

import static java.util.stream.Collectors.joining;

public class RomanNumeralsConverter {

  private final Map<Integer, String> BASE_VALUES_MAP =
    new HashMap<Integer, String>() {{
      put (1, "I");
      put (4, "IV");
      put (5, "V");
      put (9, "IX");
      put (10, "X");
      put (40, "XL");
      put (50, "L");
      put (90, "XC");
      put (100, "C");
      put (400, "CD");
      put (500, "D");
      put (900, "CM");
      put (1000, "M");
    }};

  private StringBuilder romanNumeralString;

  public RomanNumeralsConverter () {
    this.romanNumeralString = new StringBuilder();
  }

  public String convert (int arabicNumberToConvert) {
    return romanNumeral(arabicNumberToConvert);
  }

  private String romanNumeral(int arabicNumberToConvert) {
    int nearestBaseValue = findNearestBaseValue(arabicNumberToConvert);
    int numberOfBasesInTheArabicNumber = arabicNumberToConvert / nearestBaseValue;
    int numberRemainingAfterLargestBaseHasBeenDeducted =
      arabicNumberToConvert % nearestBaseValue;

    this.romanNumeralString = buildRomanNumeralString (nearestBaseValue,
      numberOfBasesInTheArabicNumber);
    return numberRemainingAfterLargestBaseHasBeenDeducted != 0 ?
      convert (numberRemainingAfterLargestBaseHasBeenDeducted) :
      this.romanNumeralString.toString();
  }

  private StringBuilder buildRomanNumeralString(int nearestBaseValue,
                                         int numberOfBasesInTheArabicNumber) {
    this.romanNumeralString.append(repeatBaseValue(nearestBaseValue,
      numberOfBasesInTheArabicNumber));
    return this.romanNumeralString;
  }

  private String repeatBaseValue (int nearestBaseValue,
                                  int numberOfBasesInTheArabicNumber) {
    return Stream.generate(() -> BASE_VALUES_MAP.get(nearestBaseValue))
      .limit(numberOfBasesInTheArabicNumber)
      .collect(joining());
  }

  private int findNearestBaseValue(int arabicNumberToConvert) {
    int nearestBaseValue = 0;
    int minimumDifference = Integer.MAX_VALUE;
    if (BASE_VALUES_MAP.get (arabicNumberToConvert) != null) {
      nearestBaseValue = arabicNumberToConvert;
    } else {
      for (Integer key : BASE_VALUES_MAP.keySet ()) {
        int differenceBetweenBaseValueMapKeyAndArabicNumber = (arabicNumberToConvert - key);
        if (differenceBetweenBaseValueMapKeyAndArabicNumber < minimumDifference
          && differenceBetweenBaseValueMapKeyAndArabicNumber > 0) {
          nearestBaseValue = key;
          minimumDifference = differenceBetweenBaseValueMapKeyAndArabicNumber;
        }
      }
    }
    return nearestBaseValue;
  }
}

Answer 1

private final Map<Integer, String> BASE_VALUES_MAP

This is missing static modifier

private StringBuilder romanNumeralString;

I don't think that should be a member, just pass it as argument where necessary. That way you code will be thread-safe, without any additional effort.

private int findNearestBaseValue(int arabicNumberToConvert) {

If you use TreeSet instead of HashSet you could just use ceiling()/floor() instead of manual iteration.

buildRomanNumeralString()

This method looks unnecessary, just inline it.

Are there Java 8 features that I should be using but not using.

I would say it is the opposite - there is no real point in using Stream in repeatBaseValue(), normal for loop would be fine.

Answer 2

First off, this probably isn't the best example to use for OOP practices. Basically standalone converters are usually static methods and don't require anything but the arguments passed to them and other static methods.

Another thing I noticed, that page you referenced has 3 algorithms and you chose what was probably the least performant of the 3. Personally I like the third one, that instead of finding the base value for the number, it loops through the base values and finds how many of the value are present.

Personally I don't like to use the variable type in the variable name. This can create maintenance problems if you decide to change the type of the variable at some point.

I'm in favor of using the more up to date Map.ofEntries to fill the map. For instance:

private static final Map<Integer, Character> BASE_VALUES = Map.ofEntries(
        entry(1, 'I'),
        entry(5, 'V'),
        entry(10, 'X'),
        entry(50, 'L'),
        entry(100, 'C'),
        entry(500, 'D'),
        entry(1000, 'M')
);

I think you can simplify things by checking for the 4 and 9 special cases instead of adding extra entries for them.

I didn't notice any checking for upper and lower bounds for the integer being passed in to your methods. Never rely on whether someone else is going to validate their data.

On a side note, using a stream to build a string is the proper way to go, however your goal in this case is to append a specific number of characters to the end of a string. I agree with user158037 that a for loop would be better in this case. Also consider that since the most times a character is repeated is 3 the difference in performance is negligible and readability would be a better goal.