Roman numerals to decimal

Question

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:

Answer 1

• 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.

Answer 2

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.