-2

I want to write a function to check if a given string in roman presentation is correct or not. They are a lot of cases of non allowed combinations : (I assume that the given string will represent a number between 1 and 3999)

  • We can't have the same character more than three times in a row : ex : IIII is false.
  • Some combinations are not allowed : DD is false ('D' + 'D' = 500 + 500 = 1000 which is M
  • We can't substract a character and add the same character just after : for example IXI is not correct event if IX is 9, it's not equal to 9 + 1
  • The most significant digits has to be at the beginning not at the middle or at the end. Ex : XM (for 1010) is false while MX is the correct one.
  • etc...

So, my idea was that rather than checking for the non allowed combinations, I will write ALL the possible allowed combinations and each time we meet a combination which is not among them, we will return false. That was my idea. The inconvenient is that my final function was very long and not really easy to understand.

For example, I wrote first a function to check for the thousands (if they exist of course), the function then returns the indexes that I will use to substring the current string to move to the next part (which will be hundreds in that case) :

private static int isThousandsValid(String str){
    int len = str.length();

    char a1 = str.charAt(0);
    char a2 = (len >= 2)? str.charAt(1) : ' ';
    char a3 = (len >= 3)? str.charAt(2) : ' ';

    if (a1 == 'M' && a2 == 'M' && a3 == 'M') //if we met that combinatin
        return 3; //we have to move after 3 digits to meet the beginning
                      //of the hundred digits
    else if (a1 == 'M' && a2 == 'M')   //same raisoning for other combinations
        return 2;

    else if (a1 == 'M') 
        return 1;

    else if (a1 == 'D' || a1 == 'C' || a1 == 'L'  || a1 == 'X' || a1 == 'V' || a1 == 'I'  )
        return 0;

    else return -1;

}

Then, I wrote the same thing for hundreds, tens and units. Example for hundreds :

private static int isHundredsValid(String str){
    if (str.isEmpty()) return 0;
    int len = str.length();

    char a1 = str.charAt(0);
    char a2 = (len >= 2)? str.charAt(1) : ' ';
    char a3 = (len >= 3)? str.charAt(2) : ' ';
    char a4 = (len >= 4)? str.charAt(3) : ' ';

    if (a1 == 'C' && a2 == 'M') 
        return 2;

    else if (a1 == 'D' && a2 == 'C' && a3 == 'C' && a4 == 'C')
        return 4;

    else if (a1 == 'D' && a2 == 'C' && a3 == 'C') 
        return 3;

    else if (a1 == 'D' && a2 == 'C') 
        return 2;

    else if (a1 == 'D') 
        return 1;

    else if (a1 == 'C' && a2 == 'D') 
        return 2;

    else if (a1 == 'C' && a2 == 'C' && a3 == 'C') 
        return 3;

    else if (a1 == 'C' && a2 == 'C') 
        return 2;

    else if (a1 == 'C') 
        return 1;

    else if (a1 == 'L'  || a1 == 'X' || a1 == 'V' || a1 == 'I'  )
        return 0;

    else return -1;     
}

Then, in my final function, I write this :

    public static boolean isValidRoman(String str){
    str = str.trim(); //remove spaces
    if (str.isEmpty()) return false;

    int index1 = isThousandsValid(str);     
    String str1 = mySubstring(str, index1);

    int index2 = isHundredsValid(str1);     
    String str2 = mySubstring(str1, index2);

    int index3  = isTensValid(str2);        
    String str3 = mySubstring(str2, index3);

    int index4 = isUnitsValid(str3);        
    String str4 = mySubstring(str3, index4);

    if (str1.isEmpty() || str2.isEmpty() || str3.isEmpty())
        return true;

    if (index1 == -1 || index2 ==-1 || index3 == -1 || index4 == -1)
        return false;

    return str4.isEmpty(); //if we still have ANOTHER character after it terminates
}

Finally "mySubstring" is just a simple function that I used to refactor and clear my code :

 private static String mySubstring(String str, int index){
    if (index == -1 ) return str;
    else 
        return str.substring(index);
}

I have please two main questions : Does this function seem correct for you? I had tested in many examples but I'm not really sure (I can't test all the 3999 possible combinations...)

Is it possible to improve it? Just to make it cleaner or more readable? Is there any easier way to check for the validity of roman number rather than write all those cases??

salamanka44
  • 904
  • 3
  • 17
  • 36
  • For reviewing working code, [CodeReview](http://codereview.stackexchange.com/) is the place to go. – Mephy Jun 11 '16 at 20:05

1 Answers1

2

I would go for the short and crazy solution and match the string using a regular expression:

public boolean isRoman(String s)
{
    return !s.isEmpty() 
           && s.matches("M{0,3}(CM|CD|D?C{0,3})(XC|XL|L?X{0,3})(IX|IV|V?I{0,3})");
}
Per Huss
  • 4,755
  • 12
  • 29
  • If your problem by any chance is homework, then you should seriously study regular expressions and make sure you can explain every part of the regular expression before submitting this solution... – Per Huss Jun 11 '16 at 20:11
  • It works but I didn't understand it well honestly. Can you explain or show me a link to understand the regular expression in java? thanks! – salamanka44 Jun 11 '16 at 20:14
  • Perhaps [wikipedia](https://en.wikipedia.org/wiki/Regular_expression) can be a starting point. There is also a [Java tutorial](https://docs.oracle.com/javase/tutorial/essential/regex/). And then you always have the [javadoc](https://docs.oracle.com/javase/7/docs/api/java/util/regex/Pattern.html) of course. Regular expressions are a good cornerstone in your professional programming toolbox! – Per Huss Jun 11 '16 at 20:21
  • I also found a [stackoverflow article](http://stackoverflow.com/questions/267399/how-do-you-match-only-valid-roman-numerals-with-a-regular-expression) with really good discussion about roman numbers and regular expressions! – Per Huss Jun 11 '16 at 20:26