Java converter from Arabic to Roman Numerals 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&lt;Integer, String&gt; BASE_VALUES_MAP =
    new HashMap&lt;Integer, String&gt;() {{
      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(() -&gt; 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 &lt; minimumDifference
          &amp;&amp; differenceBetweenBaseValueMapKeyAndArabicNumber &gt; 0) {
          nearestBaseValue = key;
          minimumDifference = differenceBetweenBaseValueMapKeyAndArabicNumber;
        }
      }
    }
    return nearestBaseValue;
  }
}
private final Map&lt;Integer, String&gt; 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.