Roman numerals to Arabic

Question

I've been learning to code for quite some time and my impression from the programming book I have is: "Keep it as simple as possible, but no simpler." I had an assignment to write a program that converts roman numerals to ints and vice versa, and I've been wondering how bad is the code that I worked on. Is it as terrible as I think it is? Is it because I'm a beginner? The code I wrote works fine, but I would very much appreciate it if you guys could show me a better and shorter way of reaching the same goal with simpler code (something that is definitely possible).

#include "Header.h"

class Roman {
public:
    string roman = "";
    int number;
    int as_int() { return number; }
    string int_to_roman(int number); 
    void think(const char& a, const char& b, const char& c, int& divided, const int & dividee); //meant to ensure the roman permissible
    void roman_to_int(Roman a);
};

void Roman::think(const char& a, const char& b, const char& c, int& divided, const int & dividee) {
    string name = "";
    int number = divided;
        number /= dividee;
        if (number < 1) return;
        if (number == 1)
            name += a;
        if (number == 2)
            name = name+a+a;
        if (number == 3)
            name = name+a+a+a;
        if (number == 4)
            name = name + a+b;
        if (number == 5)
            name = b;
        if (number == 6)
            name = name + b+a;
        if(number==7)
            name = name + b+a+a;
        if(number==8)
        name = name + b+a+a+a;
        if(number==9)
            name = name + a+c;
        if(number== 10)
            name += c;
        int z = divided;
        divided = (z - (number * dividee));
        roman += name;
}

string Roman::int_to_roman(int number) {
        if (number > 4999) error("Doesn't support numbers higher than 5000");
        think('C', 'D', 'M', number, 100);
        think('X', 'L', 'C', number, 10);
        think('I', 'V', 'X', number, 1);
        return roman;
}

void Roman::roman_to_int(Roman a){
}

ostream& operator << (ostream& os, Roman a) {
    return os << a.roman;
}

int test(char ch, int how_much) {
    if (cin.get() == ch) return how_much;
    cin.unget();
    return 0;
}

void dis(char const& a, char const& b, int& sum, int times, string& numeral) {
    sum += (1*times);
    int number = sum;
    sum += test(a, (3*times));
    if (number != sum) {
        numeral += a;
        return;
    }
    sum += test(b, (8*times));
    if (number != sum) numeral += b;
}

int get_number(Roman &a) {
    int sum = 0;
    string numeral = "";
    while (cin.get()!='\n') {
        cin.unget();
        char ch = cin.get();
        numeral += ch;
        switch (ch) {
        case'I':
            dis('V', 'X', sum, 1, numeral);
            break;
        case'V': sum += 5;
            break;
        case'X': dis('L', 'C', sum, 10, numeral);
            break;
        case'L': sum += 50;
            break;
        case'C': dis('D', 'M', sum, 100, numeral);
            break;
        case'D': sum += 500;
            break;
        case'M': sum += 1000;
            break;
        }
    }
    if (numeral != a.int_to_roman(sum)) error("Impermissible roman, try again.\n");
    return sum;
}

int main() {
    while (true) {
        try {
            while (true) {
                Roman a;
                cout << "Please write a roman numeral and press enter.\n";
                a.number = get_number(a);
                cout << a.number << ", " << a << endl;
            }
            system("pause");
            return 0;
        }
        catch (exception& e) {
            cerr << "error: \n" << e.what() << '\n ';
            //return 1; // 1 - indicates failure
        }
    }
}

As a neophyte programmer it would really mean a lot to me to know if I'm doing things well enough or whether I can do any better regarding the code's brevity and efficiency.

Edit: To the requests of some people, this is what Header.h contains:

#include <algorithm>
#include <cmath>
#include <vector>
#include <iostream>
#include <sstream>
#include <fstream>
#include <string>

using namespace std;

void error(const string& a) {
    throw runtime_error(a);
}

Answer 1

There's lots of places to improve. You'll grow accustomed to good coding habits with time.

    if (number > 4999) error("Doesn't support numbers higher than 5000");

Use {} curly braces. Always. Even when there's just one statement. It prevents a whole class of bugs during later maintenance, and it's easy to do. Also, bug: the boolean condition is off-by-one from the English diagnostic.

    think('C', 'D', 'M', number, 100);
    think('X', 'L', 'C', number, 10);
    think('I', 'V', 'X', number, 1);

First, kudos for representing the problem domain reasonably compactly. But the repetition of 'C' and 'X' is weird. Consider replacing this with a lookup table that maps letter to value.

void Roman::think(const char& a, const char& b, const char& c, int& divided, const int & dividee) {

Honestly, that's just a horrible signature. The a, b, c names are meaningless. And the next two names ignore the convention followed by math text books:

https://en.wikipedia.org/wiki/Division_(mathematics)

the dividend is divided by the divisor to get a quotient.

    int number = divided;

Yes, we know it's a number, the int declaration told us that. But what is its meaning? (Hint, the name starts with "q".) Please strive to use meaningful identifiers. And merge that 2nd /= line into the 1st line.

    if (number == 1)
        name += a;
    if (number == 2)
        name = name+a+a;
    if (number == 3)
        name = name+a+a+a;

These statements are quite similar. (In python this would simply be a * number.) Consider expressing the notion with a loop:

    if (number <= 3) {
        for (int i = 0; i < number; i++) {
            name += a;
        }
    }

At the end you have:

    if(number== 10)

This is probably not a case you should even be worrying about, as it suggests the block of code is trying to solve two problems (like dealing with 'C' and 'M') rather than solving one problem well. If you adjust how your code tackles the problem, you can guarantee that number will be within a certain range by the time it is seen by a given line of code. We call this an invariant, and it is a powerful technique for reasoning about correctness of your code.

    int z = divided;

Introducing z is a bit bizarre, compared to the straightforward approach of incorporating divided in the RHS (right hand side) of the following assignment. Definitely drop the outer parentheses, which do nothing. Consider dropping the inner parentheses as well, unless you're concerned the reader might not be familiar with C's order of operations, which will do a multiply before a subtraction.

    roman += name;

Clearly this works, side-effecting a member variable. But consider including roman in your method signature, to emphasize the return value to the reader.

int as_int() { return number; }

This works, but stylistically it's weird that you grouped this implementation in the middle of a bunch of declarations.

            Roman a;
            ...
            a.number = get_number(a);

That is an odd API, which superficially looks like OOP yet you're essentially writing procedural Fortran code in C++. A more usual API would be to send the input into a's constructor, which is responsible for putting the object into a consistent state, perhaps one that has the number already translated. For an outside caller to reach into the object and mutate its .number attribute is definitely not good OOP style.

There's more to say, but that's enough. It is good that you are working through exercises, since that is how you will learn. It is very good that you solicit criticism of your code, and that you want to improve what you write in future. Keep at it, take pride in your work, and your writing should progress nicely.

Answer 2

You've gotten a really nice answer regarding the logic, variable names, etc. -- all of the actual code. I want to comment on the structure of your program:

class Roman {
public:
    string roman = "";
    int number;
    int as_int() { return number; }
    string int_to_roman(int number); 
    void think(const char& a, const char& b, const char& c, int& divided, const int & dividee); //meant to ensure the roman permissible
    void roman_to_int(Roman a);
};

In short, I want you to really think whether you need a class or if two free functions are sufficient. Did you create a class because "everything in C++ must be a class"? If you really need a class, the class design needs a lot more thought.

Free functions

If you decide that a std::string is a good way of representing your Roman numerals, you can simplify your code significantly.

Your program would look like this:

std::string int_to_roman(int number);
int roman_to_int(std::string roman);

This accomplishes conversion between Roman and Arabic numerals, std::string of course can be directly inserted into std::cout, etc. There is very little work to do, because we use standard types directly.

A class

But std::string might not be a good abstraction, you might want to use the type system to enforce valid Roman numerals as input to a function, etc. In that case, creating a class is justified. This is more work, and requires careful planning. You want to design how you interact with your class first, before you implement its functionality. Your Roman class might look like this:

class Roman {
public:
    Roman(int);
    Roman(std::string);
    int as_int() const { return value; }
    std::string const& as_string() const { return roman; } 
private:
    int value;
    std::string roman;
    void think(...);
};

std::ostream& operator << (std::ostream& os, Roman roman) {
    return os << roman.as_string();
}

Such a class allows this type of interaction:

std::cout << "Please write a roman numeral and press enter.\n";
std::string str;
std::cin >> str;
Roman roman(str);
std::cout << "Roman number " << roman << " has numeric value " << roman.as_int() << '\n';

...which you could also do with the free functions. But it also allows for example:

void process_roman_numerals(std::vector<Roman> const& numerals);

Which is different from

void process_roman_numerals(std::vector<std::string> const& numerals);

(as you would have if you encode Roman numerals as a std::string) in that you don't need to validate the input. The Roman class would validate the input on construction (and throw if the input is wrong), so that a Roman object always contains a valid number. If the input is a simple std::string, the user could call your function with addresses, or sentences from the Bible, or anything else.

And once you have a class, you can define, for example, cast operators, such that you can use your Roman object as an input to a function that expects a std::string or an int.

Note also that the data members of Roman are declared private. If you do create a class, you need to use it to its fullest potential. Private data members allow you to guarantee that the data in your class is consistent and valid at all times. The only way to set the two data members is through the constructor. There is no way to change the numbers later. You could still create a setter function to change the value, and that function will be able to enforce the invariant (e.g. that value and roman represent the same value, and that roman is a valid Roman numeral, and that value is a positive integer in the range [1,4999], etc.)

Finally, your method think (and probably others too) are meant for internal use. It is part of the int_to_roman method. It makes no sense for it be called from anywhere else, and could be quite dangerous if your class user is able to call it and break your invariant. Thus is must be private.

Obviously a class is a more complicated beast. Not that your code is that much more complex, but you have to think about how it will be used, what methods it will need, what are its invariants, how you can protect those, etc.

Designing classes is something you'll get better at the more you do it. But only if you know to pay attention to it. I think most courses don't do a good job in this aspect. They either just tell you to write getters and setters for every single data member, or they don't discuss design at all. That's a pity.

Answer 3

In general, it seems to me that your guilty of over-thinking the problem. Basically the problem is a substitution problem. to this end a map<char,int> is a good place to start.

map<char, int> romanValues =
{
    { 'I',1 },
    { 'V',5 },
    { 'X',10 },
    { 'L',50 },
    { 'C',100 },
    { 'D',500 },
    { 'M',1000 }
};

From here a simple function, to read each character of the string and add the resultant values together, is very simple to make. Here's one that has basic validity checking:

int RomanToInt(const string& input)
{
    size_t limit = input.size();
    int retVal = 0;
    for (size_t i = 0; i < limit; i++)
    {
        auto valueIter = romanValues.find(input[i]);
        if (valueIter != romanValues.end())
        {
            int modifier = 1;
            if (((i + 1) < limit) && (valueIter->second < romanValues[input[i + 1]]))
            {
                modifier = -1;
            }
            retVal += valueIter->second * modifier;
        }
    }
    return retVal;
}

Answer 4

Remove unnecessary headers

These lines are not necessary for the header:

#include <algorithm>
#include <cmath>
#include <vector>
#include <iostream>
#include <sstream>
#include <fstream>

And this line is missing:

#include <stdexcept>

It's not a good idea to include files in a header simply because something that includes that header might want those definitions - let the code include only what it needs, and you will benefit from faster compilation. This is the case for <iostream>, for example.

For a single-file program such as this, we probably don't need a header at all - just define the one function in the main C++ source file.

Avoid using namespace

Importing all names of a namespace is a bad habit to get into, and can cause surprise when names like begin and size are in the global namespace. Get used to using the namespace prefix (std is intentionally very short), or importing just the names you need into the smallest reasonable scope.

The exceptions to this rule are namespaces explicitly intended to be imported wholesale, such as the std::literals namespaces.

Whatever you do, don't write any such using declarations in your header files - that inflicts the badness on every source file that includes your header, and may unexpectedly change the meaning of the program.

Turn up the compiler's warnings

My usual warning levels expose some questionable practices:

• multi-character char constant '\n ' in main() - you probably meant '\n';
• unused parameter a in roman_to_int() - which is never called, so it should probably be removed;
• Roman::number is not initialized by the (compiler-generated) default constructor.

Remove unreachable code:

        while (true) {
            // [snip]
        }
        system("pause");
        return 0;

The statements following the while block cannot be reached. (That is actually a good thing - because you forgot to include <cstdlib> which defines std::system(), and because the presence of a pause command on the user's PATH is completely system-dependent).

Separate the I/O from the processing

The get_number() function mixes reading from std::cin with the conversion to int. That makes it hard to read from a different file, for example, or from a GUI. Instead, read into a std::string (there's already a method called getline() provided for you), then you can write one function to process the string, regardless of where it came from.

Re-think the interface

I don't think there's a good argument to define a class for such a conversion. You probably want a simple functional interface that looks like

int from_roman(const std::string& roman);
std::string to_roman(int number);

You might consider using an unsigned type instead of plain int. (More advanced programmers will consider writing these as template functions, to allow the calling code to choose the type, but we'll leave that to a later lesson).