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\$
  • 1
    \$\begingroup\$ Why are you using floats when roman numerals represent integers? \$\endgroup\$ – David Harkness Mar 14 '13 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 Mar 16 '13 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\$ – Hugo Dozois Mar 21 '13 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\$ – Chase Mar 21 '13 at 19:32
  • \$\begingroup\$ @Chase glad I could help! \$\endgroup\$ – Hugo Dozois Mar 21 '13 at 19:36
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.

|improve this answer|||||
\$\endgroup\$
  • \$\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 Mar 14 '13 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\$ – Hugo Dozois Mar 14 '13 at 13:26
  • \$\begingroup\$ You will find good explanation in Effective Java, Joshua Bloch's book, §33 \$\endgroup\$ – cl-r Mar 14 '13 at 16:23
  • \$\begingroup\$ You can use Map<Character, Integer>tempMap = new HashMap<Character, Integer>(7, 1.0f); isntead. \$\endgroup\$ – assylias Mar 18 '13 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!

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

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.