Code not outputting correct value if input is invalid.

[romey]

I'm writing a Roman numeral to Arabic number converter.
Its mostly working - just a small hiccup.

One requirement is that if the input has an invalid character (or impossible Roman number IE Viiiv) the code should process and output and much as it can before the invalid character.

When I input an invalid character I always get 0.

I have written it so that it will check multiple arrays (thousands, hundred, tens, units) in descending order and if it finds a match to what is in the first position of the input string, it will add to a result variable which is output at the end.

Entire code at bottom, but minimum replicable is just below.
It feels like its an issue with the rfind() method. But it feels like it should work.

Any help at all will be appreciated.

#include <iostream>
#include <string>
#include <sstream>
#include <cctype>
using namespace std;
int main(int argc, char *argv[]) {
const string thousands[] = {"ZERO", "M", "MM", "MMM"};
string input;
    while (cin >> input) {
        for (int i = 0; i < input.length(); ++i) {      //Make input uppercase
            input.at(i) = toupper(input.at(i));
        }

int result = 0;
        for (int j = (sizeof(thousands) / sizeof(thousands[0])); j > 0; --j) {
            if ((input.rfind(thousands[j]) == 0)) { //if value of thousands index j is found at position 0 of input
                result += (j * 1000);
                input.erase(0, (thousands[j].length())); //Removes roman numerals from string
                break;
            }
        }
cout << result << '\n';
}

 #include <iostream>
#include <string>
#include <sstream>
#include <cctype>


using namespace std;

int main(int argc, char *argv[]) {
    const string units[] = {"ZERO", "I", "II", "III", "IV", "V", "VI", "VII", "VIII", "IX"};
    const string tens[] = {"ZERO", "X", "XX", "XXX", "XL", "L", "LX", "LXX", "LXXX", "XC"};
    const string hundreds[] = {"ZERO", "C", "CC", "CCC", "CD", "D", "DC", "DCC", "DCCC", "CM"};
    const string thousands[] = {"ZERO", "M", "MM", "MMM"};

    string input;

    while (cin >> input) {
        for (int i = 0; i < input.length(); ++i) {      //Make input uppercase
            input.at(i) = toupper(input.at(i));
        }

        int result = 0;
        for (int j = (sizeof(thousands) / sizeof(thousands[0])); j > 0; --j) {
            if ((input.rfind(thousands[j]) == 0)) { //if value of thousands index j is found at position 0 of input
                result += (j * 1000);
                input.erase(0, (thousands[j].length())); //Removes roman numerals from string
                break;
            }
        }

        for (int j = (sizeof(hundreds) / sizeof(hundreds[0])); j > 0; --j) {
            if ((input.rfind(hundreds[j]) == 0)) {
                result += (j * 100);
                input.erase(0, (hundreds[j].length()));
                break;
            }
        }

        for (int j = (sizeof(tens) / sizeof(tens[0])); j > 0; --j) {
            if ((input.rfind(tens[j]) == 0)) {
                result += (j * 10);
                input.erase(0, (tens[j].length()));
                break;
            }
        }

        for (int j = (sizeof(units) / sizeof(units[0])); j > 0; --j) {
            if (input == units[j]) {
                result += j;
                input.erase(0, (units[j].length()));
                break;
            }
        }

        cout << result << '\n';
    }
}

[dhayden]

For an array with N elements, C++ uses indices 0 to N-1, not 1 to N. Your for loops all start with j==N, so you must subtract 1 from each starting value.

Consider an input like MCMIV. When checking the thousands array, you look for M and rfind locates the one at position 2, not the one at position 0. So you want to change all your rfind()'s to find().

To check for illegal input, just check whether input is empty at the end. If it isn't empty then it was invalid.

[romey]

Thanks for your response. I greatly appreciate it and it was exactly what I was doing wrong.
The issue was indeed with the indices being incorrect as you mentioned.

Changing to find() does fix the issue you presented.

[TomCPP]

I think your solution is a little bit complicated. The main algorithm can be reduced to the following form/code:

#include <iostream>
#include <regex>
#include <unordered_map>

using namespace std;

const unordered_map<char,int> value { {'I',1},{'V',5},{'X',10},{'L',50},{'C',100},{'D',500},{'M',1000} };

int convert( string number )
{
    for( auto& letter : number ) letter = toupper(letter);
    if( !regex_match( number , regex{"^M{0,3}(CM|CD|D?C{0,3})(XC|XL|L?X{0,3})(IX|IV|V?I{0,3})$"} ) ) return -1;

    int result {0};

    for( auto iter { number.cbegin() } ; iter != number.cend() ; ++iter )
    {
        try
        {
            if( iter+1 != number.cend() )
            {
                if( value.at(*iter) < value.at(*(iter+1)) )
                {
                    result += ( value.at(*(iter+1)) - value.at(*iter) ); ++iter;
                    continue;
                }
            }
            result += value.at(*iter);
        }
        catch( const out_of_range& exept )
        {
           return -1;
        }
    }
    return result;
}

int main()
{
    string number {"MMmDCcXXIV"};
    cout << "Roman " << number << " is " << convert(number) << endl;

    return 0;
}

[seeplus]

This code has a logic error. IX is reported as -1, whereas it should be 9.

[jonnin]

I think you can do it without all that code, its just flat lookups, right?
I tested a few and it seems to work, but maybe I am missing some rule and validation would take a few more lines (I only fed it valid values).

int main()
{
    const string u[] =  {"", "I", "II", "III", "IV", "V", "VI", "VII", "VIII", "IX"};
    const string t[] =  {"", "X", "XX", "XXX", "XL", "L", "LX", "LXX", "LXXX", "XC"};
    const string h[] =  {"", "C", "CC", "CCC", "CD", "D", "DC", "DCC", "DCCC", "CM"};
    const string th[] = {"", "M", "MM", "MMM"};
	
	string x;
	cin >> x; //check valid input here if want to
	int i = x.length();
	int offset = 0;
	if(i == 4){ i--;  cout << th[x[offset++]-'0'];};  //this can be looped or cleaned up if you want.
	if(i == 3){ i--;  cout <<  h[x[offset++]-'0'];};
	if(i == 2){ i--;  cout <<  t[x[offset++]-'0'];};
	if(i == 1){ i--;  cout <<  u[x[offset++]-'0'];};
	cout << endl;
    
 }

[TomCPP]

@seeplus
Thanks for remark, the error was in validation rule, now corrected.

@jonnin
You have implemented converter from Arabic to Roman numbers.

[seeplus]

Thanks for remark, the error was in validation rule, now corrected.

Unfortunatealy invalid roman numbers are now converted as valid. eg IXL is incorrectly converted as 59 whereas it is invalid roman as roman numbers go high to low left to right and in this case the higher number is on the right.

[TomCPP]

Unfortunatealy invalid roman numbers are now converted as valid

It seems validation is a more bit complicated, that I previously thought :(
I decided to use regular expression with the formula find in the internet.

[seeplus]

@TomCPP :) :)

[jonnin]

Ah. going the other way is a pain. but there are only a few thousand of them, could do a bigger table :P