HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Reducing verbosity of RomanNumeral class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
romannumeralclassreducingverbosity

Problem

I point to my problem in the comments of my code.

I have a class RomanNumeral:

public class RomanNumeral {

    private String romaNumera;
    private String romans = "IVXLCDM";

    public RomanNumeral(String rn) {
        //sets rn to romaNumera and checks if rn is a valid roman numeral
    }

    public boolean equals(RomanNumeral rn) {
        //overrides .equals
    }

     public String toString() {
            //overrides toString();
    }

             public int getDecimalValue() {
        float[] valueOfNumeral = new float[romaNumera.length()]; 
        int result = 0;
        float calculatedRomanDecimal=.5f;
        for(int i = 0; i=0; j--){
            if (valueOfNumeral[j]<valueOfNumeral[j+1])
                result -= Math.round(valueOfNumeral[j]);
            else result += Math.round(valueOfNumeral[j]);
        }

        return result;
    }

} // class RomanNumerals


This compiles and does what I want it to do, however, I am trying to find a way to shorten the program so it doesn't require so many if statements.

Solution

First thing you can do is put them in else if statements. It doesn't reduce the code but it do at least reduce the number of case the program has to check each time.

Because right now if the first match is good, it'll still check all the other statements.

To remove the ifs:

The other thing you could do is a Map. Doing that would not really reduced the amount of code but would make the code more readable. See this topic to learn more about the load factor in the map and how to initialize it.

private static final Map ROMAN_MAP;
static
{
    // Note1 : Since you know the number of items in the map, you can initialize
    // the map directly with 7 safe "spots".
    // Note2 : We initialize it at 10 which is Roughly (N / load) + 1
    MaptempMap = new HashMap(10);
    tempMap.put('I', 1);
    tempMap.put('V', 5);
    tempMap.put('X', 10);
    tempMap.put('L', 50);
    tempMap.put('C', 100);
    tempMap.put('D', 500);
    tempMap.put('M', 1000);
    romanMap= Collections.unmodifiableMap(tempMap);
}


Then you can access it just like that :

ROMAN_MAP.get(romaNumera.charAt(i));


Changes with the use of a map :

Also, as mentioned, get rid of the floats AND of the Math.round() calls they are useless since your function returns an int anyway an that there is no possibility of displaying floating points anyway.

Your function can then be reduced to the following :

public int getDecimalValue() {
    int result = 0;
    int length = romaNumera.length();

    for(int i = 0; i < length; i++){
        int current = ROMAN_MAP.get(romaNumera.chartAt(i));
        int j = i+1;

        // Check if there is a next element and if it's bigger than current
        if(j < length && current < ROMAN_MAP.get(romaNumera.chartAt(j)){
            result -= current;
        } else {
            result += current;
        }
    }
    return result;
}


I've put { everywhere since they don't slow the process running in anyway and to me they make the code more readable and reduce the possibilities of error when refactoring.

Code Snippets

private static final Map<String, Integer> ROMAN_MAP;
static
{
    // Note1 : Since you know the number of items in the map, you can initialize
    // the map directly with 7 safe "spots".
    // Note2 : We initialize it at 10 which is Roughly (N / load) + 1
    Map<Character, Integer>tempMap = new HashMap<Character, Integer>(10);
    tempMap.put('I', 1);
    tempMap.put('V', 5);
    tempMap.put('X', 10);
    tempMap.put('L', 50);
    tempMap.put('C', 100);
    tempMap.put('D', 500);
    tempMap.put('M', 1000);
    romanMap= Collections.unmodifiableMap(tempMap);
}
ROMAN_MAP.get(romaNumera.charAt(i));
public int getDecimalValue() {
    int result = 0;
    int length = romaNumera.length();

    for(int i = 0; i < length; i++){
        int current = ROMAN_MAP.get(romaNumera.chartAt(i));
        int j = i+1;

        // Check if there is a next element and if it's bigger than current
        if(j < length && current < ROMAN_MAP.get(romaNumera.chartAt(j)){
            result -= current;
        } else {
            result += current;
        }
    }
    return result;
}

Context

StackExchange Code Review Q#23891, answer score: 8

Revisions (0)

No revisions yet.