Random addition in Roman numeral translator

Question

I need to convert roman numerals to integer values, order does not matter. (9=VIIII)

In the present I start to get a fudge factor from my code, it starts at 20 by the time that I have implemented X and reaches 530 by the time I reach M. I can't simply factor it out as while I = 531 and V=535, MDCLXVI = 1560. That and eclipse sometimes says it can`t run and other times that it can.

Here`s the code

C int romanToInt(char *s) {
  int n, k=0;
  while(k<[MAX_LINE]) {
    if(s[k]==’I’) {
      n=n++
    }
    if(s[k]==’v’) {
      n=n+5
    }
    if(s[k]==’X’) {
      n=n+10
    }
    if(s[k]==’L’) {
      n=n+50
    }
    if(s[k]==’C’) {
      n=n+100
    }
    if(s[k]==’D’) {
      n=n+500
    }
    if(s[k]==’M’) {
      n=n+1000
    }
    return n;
  }

Thanks for any help.

Answer 1

You initialize your loop counter, k, but not your accumulator, n. Initialize n=0; C assumes you know what you're doing so it doesn't automatically initialize ints to zero, like some OOP languages do. So, you could be starting with garbage, which throws off all your calculations. second: why n=n++? either user n+=1 or n++

also, a for loop would probably be better than a while loop because it forces you to initialize your counter variable (if you haven't already) and set a limit on the number of iterations, so hardly any chance of an infinite loop.

finally, instead of all those ifs, consider if/else if or a select/case.

oh, and i'm sure it's probably a typo but your return statement should be after the loop ends. You either forgot to add the extra brace before the return statement or I miscounted.

Answer 2

There are several things going on here.

int n, k=0;

You should initialise both variables to 0. At the moment, you only initialise k. n is likely to contain garbage.

while(k<[MAX_LINE]) ...

That syntax shouldn't compile. But even if it did, it is not what you want. I assume that MAX_LINE is a maximum buffer length that you use in fgets or a similar function. But the actual input is usually smaller and it contains a C string, i.e. characters terminated by a null character, '\0'. Everything after that will be garbage. Your termination condition should therefore be while (s[k] != '´\0') .... Because the value of the null character '\0' is zero, you can write this as while (s[k]) ....

if(s[k]==’I’) {
  n=n++
}

This syntax for incrementing is not legal in C. (It is a common pitfall, because it will compile.) If you want to inctement n, use plain n++. Alternatively, you can use n = n + 1 or n += 1.

if(s[k]==’v’) {
  n=n+5
}

Here, you have used the lower-case 'v', which is different from upper-case 'V'.

return n;

You return unconditionally from within the loop, i.e. after the first iteration. That's not what you want. Return the accumulated value after the loop.

Except that in your case, you'd have an infinite loop: Your position marker k never changes. You should increment k as last thing inside the loop. You could also rewrite your while as for and incrementb in the update section.

So, putting it all together:

int romanToInt(const char *s)
{
    int n = 0;
    int k = 0;

    while (s[k]) {
        if (s[k] == 'I') n++;
        if (s[k] == 'V') n = n + 5;
        if (s[k] == 'X') n = n + 10;
        if (s[k] == 'L') n = n + 50;
        if (s[k] == 'C') n = n + 100;
        if (s[k] == 'D') n = n + 500;
        if (s[k] == 'M') n = n + 1000;
        k++;
    }
    return n;
}

This will accumulate the roman numbers represented by upper-case letters (without doing the subtraction thing as in IV) and ignore everything else.