7
\$\begingroup\$

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<romaNumera.length(); i++) {

            /* My attempt at shortening the code
            for(int j = 0; j<romans.length(); j++) {

                if ( romans.charAt(j)==( romaNumera.charAt(i) ) && (j)%2==0 ) {valueOfNumeral[i] = calculatedRomanDecimal*2f;}
                if ( romans.charAt(j)==( romaNumera.charAt(i) ) && (j)%2==1 ) {valueOfNumeral[i] = calculatedRomanDecimal*5f;} 
            */
        //What I am trying to do above and what I did bellow was check which               Roman numeral was which, and assign a float value to the corresponding index in a float array. Say that I have a Roman numeral MCM. MCM is romaNumera of type string, and I compare each character in romaNumera to each character in romans. Since romaNumera.charAt(0)==romans.charAt(6), I assign valueOfNumeral[0] the value of 1000      
                //The stuff bellow works, but I feel like there is a more efficient way         

                if (romans.charAt(0)==(romaNumera.charAt(i))) {valueOfNumeral[i] = 1;}
                if (romans.charAt(1)==(romaNumera.charAt(i))) {valueOfNumeral[i] = 5;}
                if (romans.charAt(2)==(romaNumera.charAt(i))) {valueOfNumeral[i] = 10;}
                if (romans.charAt(3)==(romaNumera.charAt(i))) {valueOfNumeral[i] = 50;}
                if (romans.charAt(4)==(romaNumera.charAt(i))) {valueOfNumeral[i] = 100;}
                if (romans.charAt(5)==(romaNumera.charAt(i))) {valueOfNumeral[i] = 500;}
                if (romans.charAt(6)==(romaNumera.charAt(i))) {valueOfNumeral[i] = 1000;}

        }

        result = Math.round(valueOfNumeral[romaNumera.length()-1]);
        for (int j = valueOfNumeral.length-2; j>=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.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Why are you using floats when roman numerals represent integers? \$\endgroup\$ Commented Mar 14, 2013 at 4:56
  • \$\begingroup\$ You should add checks for invalid roman numbers. Note that even using only roman digits, not all numbers should be valid, e.g. "IIII", "DM" or "VX". \$\endgroup\$
    – Sulthan
    Commented Mar 16, 2013 at 12:23
  • \$\begingroup\$ @Chase I would like to know what do you think about the feedback you received? Anything more you'd like to know/need help with ? \$\endgroup\$ Commented Mar 21, 2013 at 19:14
  • \$\begingroup\$ @Hugo I actually learned a lot from the answer I just accepted now (for some reason I couldn't accept it before). I have no complaints, and I had already finished the project, I was just looking for better ways to do the same thing. :) \$\endgroup\$
    – Ishmael
    Commented Mar 21, 2013 at 19:32
  • \$\begingroup\$ @Chase glad I could help! \$\endgroup\$ Commented Mar 21, 2013 at 19:36

2 Answers 2

8
\$\begingroup\$

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<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);
}

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.

\$\endgroup\$
4
  • \$\begingroup\$ +1 for using map (you can now use also EnumMap). I'm not certain that valueOfNumera will be useful for good coding \$\endgroup\$
    – cl-r
    Commented Mar 14, 2013 at 13:21
  • \$\begingroup\$ @cl-r Agree with you with "valueOfNumera". I'll try update later on! (I didn't know about EnumMap, I'll take a look at it) \$\endgroup\$ Commented Mar 14, 2013 at 13:26
  • \$\begingroup\$ You will find good explanation in Effective Java, Joshua Bloch's book, §33 \$\endgroup\$
    – cl-r
    Commented Mar 14, 2013 at 16:23
  • \$\begingroup\$ You can use Map<Character, Integer>tempMap = new HashMap<Character, Integer>(7, 1.0f); isntead. \$\endgroup\$
    – assylias
    Commented Mar 18, 2013 at 22:08
1
\$\begingroup\$

Your first loop can be reduced to something like this:

String romans = "IVXLCDM";
int roman_values[] = new int[] {1,5,10,50,100,500,1000};
int result = 0;

for(int i=0;i<n;i++)
    result += roman_values[romans.indexOf(romaNumera.charAt(i))];

Also, there is NO REASON to have floating-point stuff in a Roman numeral parser. Get rid of it!

\$\endgroup\$
1
  • \$\begingroup\$ -1 : more difficult to use : how do you retrieve 'D' easily ? +1 to point out intusage. \$\endgroup\$
    – cl-r
    Commented Mar 14, 2013 at 13:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.