Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Could someone please point out the error(s) in the given code? It was downvoted on Stack Overflow without any explanation, but it seems to be working fine for me:

int value(char roman)
{
   switch(roman)
   {
        case 'I':return 1;
        case 'V':return 5;
        case 'X':return 10;
        case 'L':return 50;
        case 'C':return 100;
        case 'D':return 500;
        case 'M':return 1000;
   }
}

int getdec(const string& input)
{
  int sum=0; char prev='%';
  for(int i=(input.length()-1); i>=0; i--)
  {
    if(value(input[i])<sum && (input[i]!=prev))
    {       sum -= value(input[i]);
            prev = input[i];
    }
    else
    {
            sum += value(input[i]);
            prev = input[i];
    }
  }
  return sum;
}

This was the output received from the code:

I = 1
II = 2
III = 3
IV = 4
V = 5
VI = 6
VII = 7
VIII = 8
IX = 9
X = 10
XI = 11
XII = 12
XIII = 13
XIV = 14
XV = 15
XVI = 16
XVII = 17
XVIII = 18
XIX = 19
XX = 20
XXI = 21
XXII = 22
XXIII = 23
XXIV = 24
XXV = 25
XXVI = 26
XXVII = 27
XXVIII = 28
XXIX = 29
XXX = 30
XXXI = 31
XXXII = 32
XXXIII = 33
XXXIV = 34
XXXV = 35
XXXVI = 36
XXXVII = 37
XXXVIII = 38
XXXIX = 39
XL = 40
MMMMCMXCIX = 4999
CM = 900
XC = 90

share|improve this question
1  
Roman Numeral converter in 855 chars :-) codegolf.stackexchange.com/questions/797/… –  Loki Astari Feb 19 '13 at 2:57
    
I think IIX would break your code... –  David Anderson May 9 '13 at 2:38
add comment

2 Answers

There is a G++ compile warning (g++ -Wall):

roman.cpp: In function ‘int value(char)’:
roman.cpp:18:1: warning: control reaches end of non-void function [-Wreturn-type]

It should handle invalid inputs too. (Furthermore, it returns 9 for IIIIIIIII.)

share|improve this answer
    
Thanks. So validation checks are needed but otherwise the logic is correct? –  user1071840 Feb 19 '13 at 0:25
    
@user1071840: To be honest, I don't know. I've checked it with 5-10 different Roman numerals and it worked fine but I don't know too much about the Roman numerals-decimal numbers conversion algorithms. –  palacsint Feb 19 '13 at 0:41
    
Thank you for the help. –  user1071840 Feb 19 '13 at 1:32
add comment

If you have only valid Roman numbers you could use a simpler algorithms. (i.e IIV is invalid )

  • The symbols "I", "X", "C", and "M" can be repeated three times in succession, but no more. (They may appear more than three times if they appear non-sequentially, such as XXXIX.) "V", "L", and "D" can never be repeated. A common exception to this is the use of IIII on clocks; see below.
  • "I" can be subtracted from "V" and "X" only. "X" can be subtracted from "L" and "C" only. "C" can be subtracted from "D" and "M" only. "V", "L", and "D" can never be subtracted
  • Only one small-value symbol may be subtracted from any large-value symbol.

Just (string-)replace IV by IIII, IX by VIIII and so on. Afterwards you just have to sum the numbers from left to right.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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