patterncppMinor

Roman Numeral to Decimal Conversion

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

codereview cpp stackoverflow c++algorithm roman-numerals converting

conversionnumeralromandecimal

Problem

I'm mostly looking to get feedback on how well I've implemented the C++ language. But I would also like feedback on my algorithm. Did I make it more complex than it needs to be? Is there anything I could have done better?

Things my algorithm prevents:

IIII

IIIV

Things it does not prevent (that I'm aware of):

XICV

IXIX

Note: This uses the rules of roman numerals I was taught, and I realize that there isn't really a an accepted standard of rules for representing numbers using roman numerals.

#include 
#include 
#include 
#include  // required for tolower()

using namespace std;

int get_roman_value( char );
int roman_to_decimal( string& );

int main( int argc, char* argv[] )
{

    if( argc > 1 )
    {
        string roman(argv[1]);
        try {
            cout  digits;  // queue of the digits
    string::iterator i; // iterator to loop through the roman string

    for( i = roman.begin(); i  2 )
            throw "too many multiples";

        // check for a substraction operation
        if( !digits.empty() && current > digits.top() )
        {
            if( multiple > 0 )
                throw "cannot subtract from a multiple";
            if( last_add == digits.top() )
                throw "addition cannot follow subtraction";
            decimal+=(current-digits.top()), last_sub=digits.top(), digits.pop();
        }
        // check for multiples
        else if( !digits.empty() && current == digits.top() )
            decimal += current, multiple++;
        // regular addition operation
        else
            if( last_sub == current )
                throw "cannot add the same digit to a subtraction";
            else
                decimal+=current, multiple=0, last_add=current;

    } while( !digits.empty() );

    return decimal;

}

Solution

Well, first of all you should probably declare variables closer to usage. Not only this will compress your code a bit, it will also save you the trouble of assigning random things to them. For example int current = 0 declaration can be omitted.

You normally want to use != operator when dealing with iterators, so that you can switch containers, implementations, etc; and you'd generally want to increase the iterator without making an extra copy of same. So for example the following

for( i = roman.begin(); i < roman.end(); i++ )
    digits.push( get_roman_value(*i) );

is better written as

for( i = begin(roman); i != end(roman); ++i )
    digits.push( get_roman_value(*i) );

also, there are some obscure reasons to write begin(container) and end(container) instead of invoking member functions, but most would say it's a matter of taste.

Depending on how serious your are, you shouldn't throw strings, instead throw something that derives from exception, which is a part of STL which lives in "`

" header.

Then there's some redundancy in your

switch

, for example

case 'i': return    1; break;


the break statement is not necessary, and most programmers are very familiar with

case -> return

 pattern. Ultimately it's a matter of taste, I guess.

EDIT Answer to comment

I would prefer to remove the declaration of

int current = 0`, and instead use:

// get the current digit
int current = digits.top();
digits.pop();

Although I personally like your use of comma operator, most people emphatically do not; and encourage to separate statements on separate lines.

I also noticed you are a bit inconsistent with spaces, it's advisable to always have same rules everywhere. For example there are places where you surround the assignment operator with spaces, and then there are cases when you don't.

Code Snippets

for( i = roman.begin(); i < roman.end(); i++ )
    digits.push( get_roman_value(*i) );

for( i = begin(roman); i != end(roman); ++i )
    digits.push( get_roman_value(*i) );

case 'i': return    1; break;

// get the current digit
int current = digits.top();
digits.pop();

Context

StackExchange Code Review Q#10686, answer score: 7

Revisions (0)

No revisions yet.