Converting roman numerals to decimals c++

Question

I'm trying to convert roman numerals to decimals in c++.

So my code is supposed to convert roman to decimals but it isn't completely working.

For example, VI is 4 and IV is 6.
And MCMXLVI should yield 1946, but I am getting -998 if I go left to right, and I'm getting 0 if I go right to left.

I mainly want to know if my thinking process is right.

pseudocode:

total = 0
    max_value_so_far = 0
    for each character in the input string, going from right to left:
        if character converted to decimal >= max_value_so_far
            add character converted to decimal to total
            update max_value_so_far
        otherwise subtract character converted to decimal from total 

Code:

#include "std_lib_facilities_5.h"

string convert_string( string input){

    for(int i=0; i < input.length(); i++){
        input[i] = toupper(input[i]);
    }

    return input;
}

int roman_to_int(string RomanChars){

    RomanChars = convert_string(RomanChars);

    int total = 0;
    int max_value= 0;
    int M,D,C,L,X,V,I;
    M = 1000;
    D = 500;
    C = 100;
    L = 50;
    X = 10;
    V = 5;
    I = 1;

    double StringLength =RomanChars.length();

    for( int i = 0; i < StringLength; i++){

        if(RomanChars[i] == 'M') {
            if (M >= max_value) {
            total += M;
            max_value = M;
            }  else {
            total -= M;
            }
        }
        if(RomanChars[i] == 'D') {
            if (D >= max_value) {
                total += D;
                max_value = D;
            }  else {
                total -= D;
            }
        }
        if(RomanChars[i] == 'C') {
            if (C >= max_value) {
                total += C;
                max_value = C;
            }  else {
                total -= C;
            }
        }
        if(RomanChars[i] == 'L') {
            if (L >= max_value) {
                total += L;
                max_value = L;
            }  else {
                total -= L;
            }
        }
        if(RomanChars[i] == 'X') {
            if (X >= max_value) {
                total += X;
                max_value = X;
            }  else {
                total -= X;
            }
        }
        if(RomanChars[i] == 'V') {
            if (V >= max_value) {
                total += V;
                max_value = V;
            }  else {
                total -= V;
            }
        }
        if(RomanChars[i] == 'I') {
            if (I >= max_value) {
                total += I;
                max_value = I;
            }  else {
                total -= I;
            }
        }
    }
    return total;
}

int main() {

    string character;
    int conversion = 0;

    while(cin >> character){

         conversion = roman_to_int(character);
         cout << conversion <<endl;
    }
    return 0;
}

Answer 1

The algorithm provided by your professor is right.

Some hints:

for( double i = 0; i < RomanChars.length(); i++){

• that one loops from left to right. Wrong direction.
• double? As a looping variable? Don´t do that. (not the cause of your problem, but baaad)

The other answer already pointed out that your else's are wrong. If I were you I would think about finding something elegant to replace this copy and paste orgy.

If you would – just for the sake of inspiration – store your roman number literal values in a std::map your conversion loop would look similar to this one (the std::map in my example is std::map<char, int> conversion):

int roman_to_int(std::string RomanChars){

    RomanChars = convert_string(RomanChars);

    int total = 0;
    int max_value= 0;

    for( size_t i = RomanChars.length()-1; i != std::string::npos; --i){ 

        auto val = conversion.find(RomanChars[i]);
        if(val != conversion.end())
        {
            if(val->second >= max_value)
            {
                total += val->second;
                max_value = val->second;
            } else {
                total -= val->second;
            }
        }
    }
    return total;
}

Answer 2

For each of your if tests, you get an error due to your else block not being executed when you think it does.

        if(RomanChars[i] == 'M' && M >= max_value){
            total += M;
            max_value = M;
        } else {
            total -= M; // this got executed for each character
                        // in the input sequence which is *not* M!
        }

should be

        if (RomanChars[i] == 'M') {
            if (M >= max_value) {
                total += M;
                max_value = M;
            } else {
                total -= M;
            }
        }

And better yet:

switch (RomanChars[i])
{
case 'M':
    if (M >= max_value) {
        max_value = M;
        total += M;
    } else {
        total -= M;
    }
    break;
// ...

And better yet, refactor your code with a

#include<map>
const std::map<char, unsigned> romans_definition = {
    { 'I', 1 },
    { 'V', 5 },
    { 'X', 10 },
    { 'L', 50 },
    // ...
};