Thread: Roman Numeral Translator - Where did I go wrong?

Hi,
I'm trying to make a program that translates Roman Numerals to arabic numbers, I guess everyone should start with something like that. I've read all the beginner tutorials on this site and decided I wanted to test myself and made this.
Ofcourse I started with almost as much errors as the total amount of lines of code, but by looking them all up on google I managed to get rid of all of them. But I guess that's not the best way to make your program work, because now, without any errors or warnings, it still doesn't work.
So I would really appreciate if anyone could tell me what is causing my program not to work.

the problem is that whatever Roman Numeral I put in it returns the amount of Numerals * 1000.

So:
putting in L returns 1000,
putting in MCLXX returns 5000.

my code:

Code:

#include <iostream>

using namespace std;


class RtoA { // Class for translating a Roman numeral to an Arabic number.


public:
        int output;
        string input;
        RtoA() {}; //Constructor
        ~RtoA() {}; //Destructor
        int read(string input) { // Reads the input and returns the output.
            output = 0;
            unsigned int n = 0;
            output = addvalues(n);
            output = check(output);
            return(output);
        };


private:
        int addvalues(unsigned int n) { // Adding the values of the Numerals using recursion.
            if ( !( n > input.length() ) ) { // Bottom of the function.
                n++;
                output = addvalues(n) + givevalue(n);
                };
            if ( output == 0 ) { return(0); }
            else { return(output); };
        };


        int givevalue(int n) { // Function giving the numerals a value.
            if ( input[n] == 'M' || 'n' ) { return(1000); }
            if ( input[n] == 'D' || 'd' ) { return(500); }
            if ( input[n] == 'C' || 'c' ) { return(100); }
            if ( input[n] == 'L' || 'l' ) { return(50); }
            if ( input[n] == 'X' || 'x' ) { return(10); }
            if ( input[n] == 'V' || 'v' ) { return(5); }
            if ( input[n] == 'I' || 'i' ) { return(1); }
            else { return(0); };
        };


        int check(int output) { // Checking the order of the Roman Numerals.
            for ( unsigned int j = 1; j < input.length(); j++ ) {
                if ( givevalue(input[j]) > givevalue(input[j-1]) ) {
                    output = output - givevalue(input[j-1]);
                };
            };
            return(output);
        };
};


int main()
{
    RtoA translate;
    cout<< "Type a Roman Numeral please: \n";
    getline(cin, translate.input, '\n');
    cout<< translate.read(translate.input);
    cin.get();
}

Maybe it is just a stupid mistake but as I said I'm still a beginner,
Thanks a lot in advance!

O_o

The `n' variable in the `read' method is unnecessary.

The recursion of the `addvalues' method is broken.

The "OR" conditions in the `givevalue' method are wrong.

The `givevalue' method interface is unusual and easy to use incorrectly.

The `check' method doesn't try do what it claims.

The `check' method fails to do what it tries.

The `check' method misuses the `givevalue' method multiple times.

The `check' method should be renamed.

This doesn't need to be a class.

You shouldn't read into the class variable because you are going to pass that input as an argument to the class method.

Soma

I was hoping that it would check if the character is D or d. I guess that's not what it does?

Originally Posted by phantomotap

O_o

The `n' variable in the `read' method is unnecessary.

The recursion of the `addvalues' method is broken.

The "OR" conditions in the `givevalue' method are wrong.

The `givevalue' method interface is unusual and easy to use incorrectly.

The `check' method doesn't try do what it claims.

The `check' method fails to do what it tries.

The `check' method misuses the `givevalue' method multiple times.

The `check' method should be renamed.

This doesn't need to be a class.

You shouldn't read into the class variable because you are going to pass that input as an argument to the class method.

Soma

WOW that sounds like I really messed up, thanks for reading my code and providing me with that knowledge, I'll try and find out WHY those things are all wrong..

Originally Posted by hauzer

Nope. This checks if input[n] is equal to 'D' or if 'd' is true (ie. non zero) which it always is. The or logical comparison takes two values and if either of them is true the whole evaluation is true; hence, the statement should be written like this:

Code:

if((input[n] == 'D') || (input[n] == 'd')) { ... }

This could be simplified by first converting the letter to either upper or lower case and then comparing it to one char, and not two.

And that is just the tip of the iceberg. There are many other logical and design mistakes, some of which phantomotap has pointed out.

Thanks! I see now it's actually pretty stupid..

And would this be a correct function to add the values?

Code:

int addvalues(unsigned int n=-1) { // Adding the values of the Numerals using recursion.
            if ( n >= input.length() ) { return(0); };
            output = addvalues(n+1) + givevalue(n);
            return(output);
};

Thanks a lot for the help everyone! I figured it all out and got it working!

Originally Posted by iMalc

Not really, it's quite a common mistake. We get it here once every other week.

Note that I would delete the empty constructor and destructor. Only provide these when you actually have to do something in them.

Thanks I will remove those two