Roman Numeral to Decimal Conversion 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 &lt;- "too many multiples," better represented as IV
IIIV &lt;- "cannot add the same digit to a subtraction," better represented as VI
IVI &lt;- "addition cannot follow subtraction," better represented as V
IXX &lt;- "cannot subtract from a multiple," better represented as XIX


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


XICV &lt;- "bad format," better represented as CXIV
IXIX &lt;- "repeated subtraction," better represented as XVIII
VV &lt;- "multiple is value of existing digit," better represented as X


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 &lt;string&gt;
#include &lt;stack&gt;
#include &lt;iostream&gt;
#include &lt;ctype.h&gt; // required for tolower()

using namespace std;

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

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

    if( argc &gt; 1 )
    {
        string roman(argv[1]);
        try {
            cout &lt;&lt; roman_to_decimal(roman) &lt;&lt; endl;
        } catch( const char* e ) {
            cerr &lt;&lt; e &lt;&lt; endl;
        }

    }

    return 0;
}

int get_roman_value( char digit )
{
    switch( tolower(digit) )
    {
        case 'i': return    1; break;
        case 'v': return    5; break;
        case 'x': return   10; break;
        case 'l': return   50; break;
        case 'c': return  100; break;
        case 'd': return  500; break;
        case 'm': return 1000; break;
        default: throw "bad digit";
    }   
    return 0;
}

int roman_to_decimal( string&amp; roman )
{

    int decimal = 0;    // the final result
    int multiple = 0;   // keeps track of multiples, eg. In XXV, X would be a multiple
    int current = 0;    // the digit currently being processed
    int last_add = 0;   // the last digit that wasn't a multiple, or subtraction operation
    int last_sub = 0;
    stack&lt;int&gt; digits;  // queue of the digits
    string::iterator i; // iterator to loop through the roman string


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

    do
    {

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

        // if there are too many multiples, then throw up
        if( multiple &gt; 2 )
            throw "too many multiples";

        // check for a substraction operation
        if( !digits.empty() &amp;&amp; current &gt; digits.top() )
        {
            if( multiple &gt; 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() &amp;&amp; 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;

}
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 &lt; 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 "&lt;exception&gt;" 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.