0

I am currently working on a project that converts Roman Numerals to Arabic Numbers and vice versa. I am also responsible to implement concepts like vinculum, where if you put a bar on top of a Roman numeral, the numbers below will be multiplied by 1,000.

The problem I am having is I can get only one side working, meaning: I can either just convert from Roman Numeral to Arabic without Vinculum: ex. I = 1, II = 2 However, when this works my vinculum code does not work.

Here is a snippet of my code:

int romanToDecimal(char input[], size_t end) {

int roman = 0;

int vroman = 0;
for (int i = 0; i < strlen(input); ++i)
{
    int s1 = value(input[i]);
    int s2 = value(input[i]);

   if (input[i] == '-')
    {

        for (int j = i - 1; j >= 0; --j)
        {

            roman = (roman + value(input[j]));

        }

        roman *= 1000;



        for (int k = i + 1; k <= strlen(input); k++)
            roman += value(input[k]);

    }
    else
        roman += s1;
}

    return roman;

}

We use '-' instead of the bar on top of the characters, because we cannot do that in computer easily. So IV-, would be 4000 and XI- would be 11,000 etc...

I understand that the way I am doing the loop is causing some numbers that were converted to add twice, because if(input[i] == '-') cycles through each character in the string one at a time.

OK, so my question is basically what is the logic to get it to work? So if the string contains '-' it will multiply the number by 1000, if the string does not contain '-' at ALL, then it will just convert as normal. Right now I believe what is happening is that when "if (input[i] == '-')" is false, that part of the code still runs, how do I not get it to run at all when the string contains '-'??

jwdonahue
  • 6,199
  • 2
  • 21
  • 43
J J
  • 51
  • 8
  • By the time you've processed `IV` (assuming you've done it correctly), you should have `roman == 4`. Now if you encounter a dash, just set `roman *= 1000`. You don't need to look at the preceding characters again - you've already extracted all the information you need from them. – Igor Tandetnik Jan 31 '20 at 05:14
  • 2
    That said, your approach of processing characters one by one and simply adding up some "value" of each character can't possibly work - it can't distinguish `IV` from `VI`. The value of each character may depend on characters preceding or following. – Igor Tandetnik Jan 31 '20 at 05:17
  • You did well for a first time question here, but you really should read [ask] and [mcve], and pay more attention to properly formatting the code that you post. Make use of the preview capabilities here, before you post your question. – jwdonahue Jan 31 '20 at 05:24
  • 1
    Well I made a quick stab at fixing the formatting, but your code won't compile as it stands. Always post an [mcve]. – jwdonahue Jan 31 '20 at 05:27
  • 1
    `k <= strlen(input)` will allow `k` to equal `strlen(input)`, and that could be bad. It's going to be in bounds thanks to the null terminator so it's not fatal, but probably won't give you the results you need. – user4581301 Jan 31 '20 at 05:35
  • `for (int k = i + 1; k <= strlen(input); k++) roman += value(input[k]);` eventually calls `value('\0');`. J J why call `value()` on the _null character_? – chux - Reinstate Monica Jan 31 '20 at 08:04
  • Sorry for the late reply, I am still trying to digest all of this and reading my prof's lecture notes. I forgot that C puts a \0 at the end of the character string, that was my bad. I still haven't implemented the code for IV or when preceding character is less than proceeding. I was just confused because if I put the *1000 in the loop it will multiple by 1000 as many as the loop runs. And I am not sure how to check whether a string contains a single character. I don't believe input[i] is the way because it only checks one character at a time. – J J Jan 31 '20 at 21:24

1 Answers1

1

The posted code seems incomplete or at least has some unused (like end, which if it represents the length of string could be used in place of the following repeated strlen(input)) or meaningless (like s2) variables.

I can't understand the logic behind your "Vinculum" implementation, but the simple

roman += s1;  // Where s1 = value(input[i]);

It's clearly not enough to parse a roman number, where the relative position of each symbol is important. Consider e.g. "IV", which is 4 (= 5 - 1), vs. "VI", which is 6 (= 5 + 1).

To parse the "subtractive" notation, you could store a partial result and compare the current digit to the previous one. Something like the following:

#include <stdio.h>
#include <string.h>

int value_of(char ch);

long decimal_from_roman(char const *str, size_t length)
{
    long number = 0, partial = 0;
    int value = 0, last_value = 0;
    for (size_t i = 0; i < length; ++i)
    {
        if (str[i] == '-')
        {
            number += partial;
            number *= 1000;
            partial = 0;
            continue;
        }
        last_value = value;
        value = value_of(str[i]);
        if (value == 0)
        {
            fprintf(stderr, "Wrong format.\n");   
            return 0;
        }
        if (value > last_value)
        {
            partial = value - partial;
        }
        else if (value < last_value)
        {
            number += partial;
            partial = value;
        }
        else
        {
            partial += value;   
        }
    }   
    return number + partial;
}

int main(void)
{
    char const *tests[] = {
        "I", "L", "XXX", "VI", "IV", "XIV", "XXIII-",
        "MCM", "MCMXII", "CCXLVI", "DCCLXXXIX", "MMCDXXI", // 1900, 1912, 246, 789, 2421
        "CLX", "CCVII", "MIX", "MLXVI"                     // 160, 207, 1009, 1066 
    };
    int n_samples = sizeof(tests) / sizeof(*tests);

    for (int i = 0; i < n_samples; ++i)
    {
        long number = decimal_from_roman(tests[i], strlen(tests[i]));
        printf("%12ld %s\n", number, tests[i]);
    }

    return 0;
}

int value_of(char ch)
{
    switch (ch)
    {
        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;
        default:
            return 0;
    }
}

Note that the previous code only checks for wrong characters, but doesn't discard strings like "MMMMMMMMMMIIIIIIIIIIIIIV". Consider it just a starting point and feel free to improve it.

Bob__
  • 12,361
  • 3
  • 28
  • 42
  • Hi, I am still relatively new to CC. May I ask what does the sizeof(tests)/sizeof(*tests) mean? I am guessing that it gets the length of the tests[] array and the * is a pointer. What does the pointer do in this case exactly? That's why in my code, I didn't really use it. But I know it is an important concept to understand for C programming. – J J Feb 01 '20 at 00:00
  • @JJ `sizeof(tests)/sizeof(*tests)` gets the number of elements in an array. `sizeof(tests)` provides the size of `tests` in bytes. `sizeof(*tests)` provides the size of the first value (and since they are all the same type, the size of any value) in `tests`in bytes. If you are compiling to the most recent C++ standards you can use the `std::size` function to compute the number of elements with a little less fuss. Note that due to [array decay](https://stackoverflow.com/questions/1461432/what-is-array-decaying) this will not work with an array passed into a function. – user4581301 Feb 01 '20 at 01:00
  • @JJ I posted a C program, despite your question beeing also tagged as C++, due to the snippet you posted. If you are primarly interested in C++, please edit the question to add that info. Back to your comment, yes, I used that pattern to "automatically" get the size of the array and no, you don't have to use it if you haven't alredy learned how pointers work. I could have explicitly used the size, (e.g. `const char *tests[16] = ...`) and then pass that number, but it must be correct (and updated if you decide to add another test) or add a NULL-terminator as a sentinel to stop the loop. – Bob__ Feb 01 '20 at 09:39