16
\$\begingroup\$

This is a program for converting Roman numerals to their decimal equivalents. There must be a better way of distinguishing between, for example, 'IV' and 'VI', than what I have currently written.

#include <iostream>
#include <string>
using namespace std;


class romanType{
public:
    romanType();
    romanType(string s);
    ~romanType();

        void setRoman(string); //Set the Roman numeral from user entries.
        int romanToDecimal(); //Convert the Roman numeral(string) to Decimal value.
        void printDecimal(); //Display the decimal value.
        void printRoman(); //Display the Roman numeral value.
private:
    string romanNum;
    int decimalNum = 0;

};

romanType::romanType()
{
    romanNum = 1;
}

romanType::~romanType()
{
}

void romanType::setRoman(string troll)
{
    romanNum = troll;
}

int romanType::romanToDecimal()
{
    for (int i = 0; i < romanNum.length(); i++)
    {
        if (romanNum[i] == 'I')
            decimalNum++;
        if (romanNum[i] == 'V')
        {
            if (i > 0 && romanNum[i - 1] == 'I')
                decimalNum -= 2;
            decimalNum += 5;
        }
        if (romanNum[i] == 'X')
        {
            if (i > 0 && romanNum[i - 1] == 'I')
                decimalNum -= 2;
            decimalNum += 10;
        }
        if (romanNum[i] == 'L')
        {
            if (i > 0 && romanNum[i - 1] == 'X')
                decimalNum -= 20;
            decimalNum += 50;
        }
    }
    cout << decimalNum << endl;
    return decimalNum;
}




int main()
{
    string numerals;
    do{
        cout << "Enter roman numerals: ";
        cin >> numerals;

        romanType Rom;
        Rom.setRoman(numerals);
        Rom.romanToDecimal();

    } while (true);
    system("PAUSE");

}

Sample output:

Enter roman numerals: IX
9
Enter roman numerals: XI
11
Enter roman numerals: XIX
19
Enter roman numerals: XX
20
Enter roman numerals: LVIII
58
Enter roman numerals: LXXXVIII
88
Enter roman numerals: LXVI
66
Enter roman numerals: LXIV
64
Enter roman numerals:
\$\endgroup\$
1
  • 1
    \$\begingroup\$ perhaps you should look at Enum \$\endgroup\$ Nov 15, 2013 at 21:49

2 Answers 2

18
\$\begingroup\$
  • Try not to use using namespace std and system("PAUSE").

    The former is okay for shorter programs or toy programs, or when used in local scope as opposed to global (such as within a function).

    The latter is best avoided as it is problematic while safer and more portable alternatives are in existence, such as std::cin.get(). It's also non-portable and only works with Windows.

  • In certain environments (or depending on personal preference), new user-defined types should be capitalized and objects should be lowercase.

    NewType objOfNewType;
    
  • Prefer an initializer list here:

    NewType::NewType() : dataMember1(/* value */) {}
    

    Although this may not make a huge difference in your code, it's still good to know about it when writing larger programs. It is best when initializing objects (especially when the arguments are of the same name) and necessary when initializing constants.

  • As you're not using the destructor, you can leave it out. The compiler will make one for you that does any of the default cleanup.

  • You shouldn't do this in the class declaration:

    int decimalNum = 0;
    

    You should instead put that into the initializer list as mentioned above:

    // entries are separated by commas
    // can also be listed on separate lines
    className::className() : member1(0), member2(0) {}
    
  • You have declared printRoman(), but have not defined it. Better yet, overload operator<< for your class and leave out that empty declaration.

    Declare in header:

    friend std::ostream& operator<<(std::ostream&, Class const&);
    

    Define in source:

    std::ostream& operator<<(std::ostream& out, Type const& obj)
    {
        return out << obj.typeMember;
    }
    

    Print it out:

    NewType objOfNewType;
    std::cout << objOfNewType;
    
  • int is not a good loop counter type here:

    for (int i = 0; i < romanNum.length(); i++) {}
    

    romanNum is of type std::string, so you should use std::string::size_type. This will ensure that you can loop through any string size while not being limited by int's size.

    Better yet, use its iterators instead (using a range-based for-loop in C++11):

    for (auto& iter : romanNum)
    {
        if (iter == 'I')
            decimalNum++;
    }
    

    Another alternative (if you don't have C++11 and cannot use the above):

    for (std::string::iterator = romanNum.begin(); iter != romanNum.end(); ++iter)
    {
        if (*iter == 'I')
            decimalNum++;
    }
    
  • Prefer not to pass-by-value here:

    void romanType::setRoman(string troll)
    {
        romanNum = troll;
    }
    

    As you're not modifying troll, prefer to pass by constant reference (const and &). This will avoid unneeded copying, thereby also increasing performance. It's best only for non-native types (such as std::string), so disregard it for any of the native ones.

    Example (in different forms):

    void func(const std::string& str) {}
    

    void func(std::string const& str) {}
    
  • do-while loops are usually considered less-readable than while loops. However, you don't need a while loop here as that would require adding another user input before the loop.

    Instead, I'd recommend the "infinite loop" for (;;):

    for (;;)
    {
        std::cout << "This message will print forever!\n";
        std::cout << "But please do end this eventually!\n\n";
    
        bool keepGoing = true;
        if (!keepGoing) break; // or return
    }
    

    As described, this will keep looping until the loop is ended (usually with a break, return, or return X if non-void). You'll just need a conditional that will decide when the loop should end.

  • Prefer to use variables as close in scope as possible:

    std::cout << "Input: ";
    int input;
    std::cin >> input;
    
  • This is unneeded:

    romanType Rom;
    Rom.setRoman(numerals);
    

    You're invoking the default destructor (no arguments) while calling the mutator.

    Instead, the object should be constructed right away with numerals:

    romanType Rom(numerals);
    

    Beyond that, you won't need a mutator in your program. They can also be bad for encapsulation, but that's another topic. For now, focus on proper object construction while only using necessary implementations. Keep your code as concise as possible.

    Note that my previous point about the initializer list was for example purposes. In this case, you'll need to add an argument to the list.

    NewType::NewType(ArgType arg) : member1(arg) {}
    

    With this in place:

    NewType objOfNewType(5); // will compile
    NewType objOfNewType;    // will not compile
    

    In your program, the latter should still happen as the conversion process shouldn't work with no input given to the object. You can also keep your existing default constructor, allowing the latter to still work. But there may be no need for a default value if you want the user to give input.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks! I realized it was an infinite loop, I did it intentionally because I was in the process of debugging something. The final program will ask the user if he/she wants to convert another numeral. \$\endgroup\$ Nov 16, 2013 at 0:45
  • \$\begingroup\$ @Justin: That's fine. It does still help to know about the different loops when putting them to real use. \$\endgroup\$
    – Jamal
    Nov 16, 2013 at 0:51
  • \$\begingroup\$ You have suggested to do for (;;) { ... if (!keepGoing) break; } - if you do have a flag then wouldn't it be better to do while(keepgoing) { ... }? \$\endgroup\$
    – ChrisWue
    Nov 17, 2013 at 8:19
  • \$\begingroup\$ @ChrisWue: Yes, perhaps. This was mainly an example, and I assume the OP would just need a simple loop like this. \$\endgroup\$
    – Jamal
    Nov 17, 2013 at 8:48
8
\$\begingroup\$

Jamal has some good comments on general coding style. To address the other part of your question though, here's an alternative way of implementing the conversion from a Roman Numeral string to an integer which is more general and also cleaner IMO:

int fromRoman(const string& x) {
    auto first = crbegin(x);
    const auto last = crend(x);

    auto decimalDigitFromRoman = [&](char unit, char five, char ten) {
        int num = 0;
        for (; first != last && *first == unit; ++first) ++num;
        while (first != last && (*first == ten || *first == five)) {
            num += *first == ten ? 10 : 5;
            for (++first; first != last && *first == unit; ++first) --num;
        }
        return num;
    };

    int num = 0, pow = 1;
    for (auto syms : {"IVX", "XLC", "CDM"}) {
        num += decimalDigitFromRoman(syms[0], syms[1], syms[2]) * pow;
        pow *= 10;
    }

    return num;
}

Live version.

This function handles the "standard" forms and also most of the "alternative" forms described on the Wikipedia page.

[Edited to add explanation below]

There's a few observations that lead to the approach used here.

First, we can look at a Roman numeral as a special kind of decimal number where the 'digits' can be 0-n characters long and 'digits' of different powers are written with different symbols. So the number 321 (3x100 + 2x10 + 1x1) is written CCC'XX'I (3x100 + 2x10 + 1x1) (we don't write the separating 's). For the decimal digits 0-9 in least significant position the Roman numeral 'digits' are: ''(0)(an empty string), 'I'(1), 'II'(2), 'III'(3), 'IV'(4), 'V'(5), 'VI'(6), 'VII'(7), 'VIII'(8), 'IX'(9).

Second, 'digits' of each power of 10 are written the same way, just using different symbols. For each power of 10, we have a 'unit' symbol, a 'five' symbol and a 'ten' symbol. For 10^0 these are I,V,X, for 10^1 they are X,L,C, for 10^2, C,D,M. The system doesn't extend to 10^3.

Third, to parse a 'digit' forwards is tricky since we don't know for a given 'unit' symbol whether it is additive or subtractive without looking ahead (this is the problem that makes your code a bit tricky). Parsing backwards however we only need our previously seen context to interpret a symbol. If we see a 'five' or 'ten' symbol as our first symbol in a digit parsing backwards, subsequent 'unit' symbols for our current power are subtractive. Otherwise 'unit' symbols are additive until we hit a non-unit which marks the beginning of a subsequent 'digit'.

We can now understand the code above. The lambda decimalDigitFromRoman parses a single Roman numeral 'digit' given the symbols for 'unit', 'five' and 'ten' by stepping backwards through the string using a reverse iterator. The loop below the lambda parses a sequence of Roman numeral 'digits' by calling the lambda in sequence for each set of symbols and multiplying by the corresponding power of 10. The sum is the integer value of the Roman numeral string.

These rules as expressed in the code handle most of the edge cases automatically. Alternative forms like 'IIII' rather than 'IV' for 4 conveniently just fall out of the same rules when expressed appropriately. When we get to the 1000s we can just keep adding Ms to the beginning of the number.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.