Roman Numeral To Decimal

Question

Trying to implement a very simple Roman Numeral to Decimal converter but can't seem to figure out a way for the program to return -1 if any non-roman numeral characters are in the string. This is what I have so far.

#include <stdio.h>
#include <ctype.h>

int convertFromRoman(const char *s)
{

int i = 0;
int total = 0;

while (s[i] != '\0') {

    if (isalpha(s[i]) == 0) {
        return -1;
    }

    if (toupper(s[i]) == 'I') {
        total += 1;
    }

    if (toupper(s[i]) == 'V') {
        total += 5;
    }

    if (toupper(s[i]) == 'X') {
        total += 10;
    }

    if (toupper(s[i]) == 'L') {
        total += 50;
    }

    if (toupper(s[i]) == 'C') {
        total += 100;
    }

    if (toupper(s[i]) == 'D') {
        total += 500;
    }

    if (toupper(s[i]) == 'M') {
        total += 1000;
    } else {
        return -1;
    }

    i++;
}

if (total == 0) {
    return -1;
}

return total;
}



int main()
{
    printf("%d\n", convertFromRoman("XVII"));
    printf("%d\n", convertFromRoman("ABC"));
}

The first one should return 17 and the second one should return -1. However they both return -1 but if I remove the else statement, the first one returns 17 and the second one returns 100.

Any help is appreciated.

We don't solve homework for you. Probably not a good idea to have your first line as `//Homework` — Charlie Fish, Aug 11 '16 at 03:27
Thanks for your comment @CharlieFish. Wasn't asking anyone to solve it! Was just looking for some advice on what I was missing/reference me to some documentation. Just put the homework in there so I didn't get a full answer as that's not a good way to learn! — Mikey, Aug 11 '16 at 03:29
You should `return 0;` from `main` btw. (And technically use `int main(void)` if not using args.) — RastaJedi, Aug 11 '16 at 03:31
@RastaJedi Thanks man. Slipped my mind when writing quick tests :) — Mikey, Aug 11 '16 at 03:33
Why are you using isalpha? This test does nothing for you - you have a complete set of characters you want to process — Tibrogargan, Aug 11 '16 at 03:33
@Tibrogargan I wanted to make sure the string being passed in was of letters and nothing else. Soon as something that is not alpha is in the string, it is not a valid roman numeral and therefore should not be processed. Unless I am missing something in my understanding? — Mikey, Aug 11 '16 at 03:34
Off topic, but don't keep calling `toupper(s[i])` - do it once and reuse the results. Just imaging `toupper()` is some complex thing that takes 5 seconds, and you are calling it over and over with the same data... — John3136, Aug 11 '16 at 03:34
@RastaJedi That's not strictly true. The C standard says `main` implicitly returns 0 if it does not have an explicit return. See the C11 standard or [What should main() return in C and C++?](http://stackoverflow.com/questions/204476/what-should-main-return-in-c-and-c) — kaylum, Aug 11 '16 at 03:34
@John3136 Thanks for that John. Very good point. Have amended my code :) — Mikey, Aug 11 '16 at 03:35
@BLUEPIXY thanks for the comment. This is a very simple program in which we only have to add up a string of letters and not worry about all of the roman numeral idioms. — Mikey, Aug 11 '16 at 03:36
@kaylum I was referring to C89... especially since that's what's portable to M$. Sorry for not being specific... I just always use that so I didn't really think twice. — RastaJedi, Aug 11 '16 at 03:36
@KeithNicholas Yes have just done that now and seen my silly beginner mistake! Thanks :) — Mikey, Aug 11 '16 at 03:38
@BLUEPIXY Romans of 2000 years ago rarely consider IV to be 4, usualy they considered it 6. More info: https://en.wikipedia.org/wiki/Subtractive_notation — chux - Reinstate Monica, Aug 11 '16 at 03:43
@kaylum and what gcc uses by default (well technically gnu89 I guess it is?) — RastaJedi, Aug 11 '16 at 03:43
you should also consider changing your program so it does toupper once, making a function that returns a single letters value, and explore the wonder of "switch" which is basically a big if else if else — Keith Nicholas, Aug 11 '16 at 03:47
@Mikey you have a set of known values that you intend to test against (you could be using a switch). Anything outside that set is incorrect. Why add two tests for incorrectness? — Tibrogargan, Aug 11 '16 at 03:53
@RastaJedi See https://en.wikipedia.org/wiki/Roman_numerals#Alternative_forms — chux - Reinstate Monica, Aug 11 '16 at 04:09

score 4 · Accepted Answer · answered Aug 11 '16 at 03:39

4

Change if() if() if() else to if() else if () else if() else

   if (toupper(s[i]) == 'I') {
        total += 1;
    }

    else if (toupper(s[i]) == 'V') {
        total += 5;
    }

    else if (toupper(s[i]) == 'X') {
        total += 10;
    }

    ....

    else if (toupper(s[i]) == 'M') {
        total += 1000;
    } else {
        return -1;
    }

answered Aug 11 '16 at 03:39

chux - Reinstate Monica

143,097
13
135
256

Or if you want OP you can use `switch (toupper(s[i]))` to make things a little simpler. – RastaJedi Aug 11 '16 at 03:47
1

@RastaJedi There are _many_ potential improvements to OP's code. For me, I would do something along [@Tibrogargan](http://stackoverflow.com/a/38886941/2410359). OP was having trouble with `if` `else if` `else`, so this answers focuses on that. BTW it should be `switch (toupper((unsigned char) s[i]))` to avoid UB should `s[i]` be negative. – chux - Reinstate Monica Aug 11 '16 at 03:52
Yea I was just looking at his answer; it's pretty nifty. That's good to know... I only quickly opened the man and saw the prototype as `int toupper(int c);`, but I guess a lot of functions that work with characters specify them as `int` as well, don't they. – RastaJedi Aug 11 '16 at 03:53
@RastaJedi C11dr 7.4 discusses character handling functions including `isalpha()`, `toupper()`, etc. "In all cases the argument is an `int`, the value of which shall be representable as an `unsigned char` or shall equal the value of the macro `EOF`. If the argument has any other value, the behavior is undefined." This is why I recommended the `(unsigned char)` cast. – chux - Reinstate Monica Aug 11 '16 at 04:00
Yeah I see it in my man now it says something along the same lines as that, I just forgot for a sec that even things like `strchr()` or something that expects a char value has `int` in its prototype as well. (How come `scanf()` complains if you try to use an int argument but it won't the other way around? Is that just the way default arg promotions work?) Too bad I can't edit my original comment :P. – RastaJedi Aug 11 '16 at 04:02
@RastaJedi `strchr()` is very useful for this type of problem. Important to watch out for `strchr(string, 0)` which matches the the null character of `string`. Do not expect `toupper(s[i])` in Tibrogargan answer to return 0 though. – chux - Reinstate Monica Aug 11 '16 at 04:07
I'm trying to implement it using a switch statement as well but am struggling to make it work. It keeps defaulting to -1. I can post what I have written if that helps? – Mikey Aug 11 '16 at 04:17
@Mikey SO works best as _one_ question at a time. Suggest making a new post (with a link back to this one for reference) posing the new issue about the `switch`. Tip: enable all warnings in your compiler. BTW: Suspect you are missing `break`. – chux - Reinstate Monica Aug 11 '16 at 14:09
@Mikey just a thought on the nature of roman numerals. You're using -1 to indicate an error, but you could be using 0 since it cannot be represented by Roman numerals. – Tibrogargan Aug 11 '16 at 20:54
@Tibrogargan Assignment spec states we must return -1 if a value of 0 is returned somehow as it is not a valid roman numeral as you have pointed out. Hence the if total == 0 check :) – Mikey Aug 12 '16 at 02:16

Tibrogargan · Answer 2 · 2016-08-11T03:48:35.520

Not really an answer, just a bit of fun/alternate way of looking at the problem. It does solve the problem if you're not considering ordering just adding "digit" values.

char *romanNumerals = "IVXLCDM";
int values[] = { 1, 5, 10, 50, 100, 500, 1000 };

int convertFromRoman(const char *s) {
    int val = 0;
    for (int i = 0; s[i]; i++) {
        char *idx;
        if (NULL == (idx = strchr(romanNumerals, toupper(s[i])))) {
            return -1;
        }
        val += values[idx - romanNumerals];
    }
    return val;
}

Roman Numeral To Decimal

2 Answers2