Roman Numeral to Integer Conversion Problem

Question

I am so close to solving this roman numeral to integer problem. However, in my if statement for when character equals M, I am getting an error thrown when declaring my previous variable when the input is MCMXCIV for example. Because there is nothing before M, it is throwing an out-of-bounds error. How can I fix this?

#include <iostream>
#include <iomanip>
#include <cmath>
#include <string>

using namespace std;

//Character to search and add to the integer
char character;

//The integer value that is needed to add up and output the corresponding value
int integer = 0;

//One of the test runs and what will be needed for every special case

int main()
{
//Roman numeral given
string input;

//Prompt for user to enter the roman numeral integer
cout << "Enter the roman numeral you want to convert into a number: " << endl;
cin >> input;

cout << input << " is what you are wanting to convert." << endl;
    
        
//Read through the string that is being inputted then assign values to the overall integer
for (int i = 0; i < input.length(); i++)
{
character = input.at(i);
cout << "This is the character that is being read right now: " << character << endl;
  
    
   
    
    //Arithmitic for when the character is found and the corresponding value needs to be added
    if(character == 'I')
    {
        integer+=1;
        cout << "Integer value now: " << integer << endl;
    }
    
    else if(character == 'V')
    {
        char previous = input.at(i-1);
        integer+=5;
        
        if(character == 'V' && previous == 'I')
        {
            integer = integer - 2;
            
        }
        cout << "Integer value now: " << integer << endl;
    }
   else if(character == 'X')
    {
        integer+=10;
        cout << "Integer value now: " << integer << endl;
    }
    
   else if(character == 'L')
    {
        integer+=50;
        cout << "Integer value now: " << integer << endl;
    }
    
   else if(character == 'C')
    {
        integer+=100;
        cout << "Integer value now: " << integer << endl;
    }
   else if(character == 'D')
    {
        integer+=500;
        cout << "Integer value now: " << integer << endl;
    }
    
   else if(character == 'M')
    {
        char previous = input.at(i-1);
        integer+=1000;
        if(character == 'M' && previous == 'C')
        {
            integer -= 200;
        }
        cout << "Integer value now: " << integer << endl;
    }
    
    
}

        cout << "The integer value is: " << integer << endl;
    }

Answer 1

First of all: Good that you used the at() function. So you could detect the "out of bounds" problem.

in char previous = input.at(i - 1);, variable "i" could be 0 and you then try to access array element "-1", which is of course out of bounds for your use case.

So, you need an additional check, if "i" greater then 0, before subtracting.

---

But in general, your approach is too complicated. You can make your life easier, by analyzing or reading, how roman numerals are defined. Look for example here. And please read especially about the "subtractive notation".

You already noticed that but, unfortunately, your implementation is not always following that rule. You made the check only for "M" and "V". But basically, you need to do that for all literals (except "I").

You can boil down this to the rule:

If a literal before a following literal is less, then use the subtrative form. Or, even better, you can read from right to left and finally say:

"If the current literal is less than the follwoing, then use the subtractive form."

And what is the subtractive form? We can simply add the negative number. Example, using number 94 which is "XCIV". We start summing up from the right:

• Start. Begin from right. Initialize sum with rightmost value: Looking at 'V': sum = 5
• Next: Read 'I'. Check, if this is less than the following literal 'V'. Yes, it is. So, use subtractive form. Add the negative. Now sum = sum + (-1), sum = 4
• Now: Read 'C'. Check, if this is less than the following literal 'I'. No, it is not. So, simply add the positive value. Now sum = sum + 100, sum = 104
• Next: Read 'X'. Check, if this is less than the following literal 'C' . Yes, it is. So, use subtractive form. Add the negative. Now sum = sum + (-10), sum = 94

So, this is now a very simply algorithm. We will convert a roman literal (one letter) to a integer and then build a sum with positive or negative values.

One of many many potential implementations could look like this:

#include <iostream>
#include <string>

int convert(char romanLiteral) {
    switch (romanLiteral) {
    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;
    }
}

int romanLiteralStringToInteger(const std::string& romanLiteralString) {

    // Sanity check:
    if (romanLiteralString.empty()) return 0;

    // Get length of input string
    int lengthOfRomanLiteralString = static_cast<int>(romanLiteralString.length());

    // Initialize sum with rightmost value
    int sum = convert(romanLiteralString[lengthOfRomanLiteralString-1]);

    // Now iterate over the string form right to left
    for (int i = lengthOfRomanLiteralString - 2; i >= 0; --i) {

        // Check if this literal is less than the following
        if (convert(romanLiteralString[i]) < convert(romanLiteralString[i+1]))
            sum -= convert(romanLiteralString[i]);
        else
            sum += convert(romanLiteralString[i]);
    }
    return sum;
}
int main() {
    std::string romanNumber = "XCIV";

    std::cout << romanNumber << " --> " << romanLiteralStringToInteger(romanNumber) << '\n';
}

---

In C++ you would probably use associative containers like std::map or std::unordered_map for converting one literal to a number. And maybe a ternary operator, instead of an if.

Then the problem could be implemented like the following:

#include <iostream>
#include <string>
#include <unordered_map>

int romanLiteralStringToInteger(const std::string& romanLiteralString) {

    if (romanLiteralString.empty()) return 0;

    std::unordered_map<char, int> T = { { 'I' , 1 }, { 'V' , 5 }, { 'X' , 10 }, { 'L' , 50 }, { 'C' , 100 }, { 'D' , 500 }, { 'M' , 1000 } };
    int sum = T[romanLiteralString.back()];

    for (int i = romanLiteralString.length() - 2; i >= 0; --i)
        sum += (T[romanLiteralString[i]] < T[romanLiteralString[i + 1]] ? -T[romanLiteralString[i]] : T[romanLiteralString[i]]);
    
    return sum;
}
int main() {
    std::string romanNumber = "XCIV";

    std::cout << romanNumber << " --> " << romanLiteralStringToInteger(romanNumber) << '\n';
}

---

And the hardcore solution with a stateful lambda.

#include <iostream>
#include <string>
#include <unordered_map>
#include <numeric>
#include <iterator>

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

int main() {

    std::string romanNumber = "XCIV";
    std::cout << std::accumulate(std::next(romanNumber.rbegin()), romanNumber.rend(), ARTI[romanNumber.back()], [&, next = ARTI[romanNumber.back()]](const int s, const char c) mutable {
        int sum = s + (ARTI[c] < next ? -ARTI[c] : ARTI[c]); next = ARTI[c]; return sum; });
}