2
\$\begingroup\$

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;
  }
}
\$\endgroup\$
  • \$\begingroup\$ An Arabic number is a number using Arabic characters and digits such as "12" which is build from digit '1' and a digit '2'. What you pass is a 32 bit value that makes up a number in Java called int. It's not Arabic nor Roman. Integer.toString(int) returns Arabic. \$\endgroup\$ – Maarten Bodewes Apr 23 at 20:11
1
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for your review. I will make appropriate changes. I only got one question re the following comment: "here is no real point in using Stream in repeatBaseValue(), normal for loop would be fine" - everyone else who looked at my code, including Intellij, is saying that using loop to build a string is a really bad practice. Could I ask for pointers on how to decide when is it good to use Stream and when it is good to use the for loop? \$\endgroup\$ – Igor Ryabchuk Sep 25 '18 at 12:29
  • \$\begingroup\$ I think that calling a public method convert is also not a great idea, why not call the private method romanNumeral? And always document recursive calls, they are a common place to introduce errors such as stack overflows. \$\endgroup\$ – Maarten Bodewes Apr 23 at 20:48
1
\$\begingroup\$

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.

\$\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.