Like Tree2Likes
  • 2 Post By hauzer

Roman Numeral Translator - Where did I go wrong?

This is a discussion on Roman Numeral Translator - Where did I go wrong? within the C++ Programming forums, part of the General Programming Boards category; Hi, I'm trying to make a program that translates Roman Numerals to arabic numbers, I guess everyone should start with ...

  1. #1
    Registered User
    Join Date
    Oct 2011
    Posts
    6

    Unhappy 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!

  2. #2
    Programming Wraith GReaper's Avatar
    Join Date
    Apr 2009
    Location
    Greece
    Posts
    1,624
    For starters, what do you think "(input[n] == 'D' || 'd')" does?
    Devoted my life to programming...

  3. #3
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    4,230
    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

  4. #4
    Registered User
    Join Date
    Oct 2011
    Posts
    6
    I was hoping that it would check if the character is D or d. I guess that's not what it does?

  5. #5
    Registered User
    Join Date
    Oct 2011
    Posts
    6
    Quote Originally Posted by phantomotap View Post
    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..

  6. #6
    Registered User
    Join Date
    Aug 2008
    Location
    Belgrade, Serbia
    Posts
    163
    Quote Originally Posted by M-Nikz View Post
    I was hoping that it would check if the character is D or d. I guess that's not what it does?
    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.
    M-Nikz and rags_to_riches like this.
    Vanity of vanities, saith the Preacher, vanity of vanities; all is vanity.
    What profit hath a man of all his labour which he taketh under the sun?
    All the rivers run into the sea; yet the sea is not full; unto the place from whence the rivers come, thither they return again.
    For in much wisdom is much grief: and he that increaseth knowledge increaseth sorrow.

  7. #7
    Registered User
    Join Date
    Oct 2011
    Posts
    6
    Quote Originally Posted by hauzer View Post
    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);
    };

  8. #8
    Registered User
    Join Date
    Oct 2011
    Posts
    6
    Thanks a lot for the help everyone! I figured it all out and got it working!

  9. #9
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,301
    Quote Originally Posted by M-Nikz View Post
    Thanks! I see now it's actually pretty stupid..
    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.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  10. #10
    Registered User
    Join Date
    Oct 2011
    Posts
    6
    Quote Originally Posted by iMalc View Post
    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

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Roman Numeral Counter
    By luigiramez12 in forum C Programming
    Replies: 11
    Last Post: 03-30-2011, 02:12 PM
  2. roman numeral conversion
    By clarky101 in forum C Programming
    Replies: 8
    Last Post: 12-09-2007, 06:13 PM
  3. Roman Numeral^^
    By only1st in forum C++ Programming
    Replies: 6
    Last Post: 04-19-2007, 10:58 AM
  4. Roman Numeral...T_T
    By only1st in forum C++ Programming
    Replies: 12
    Last Post: 04-12-2007, 10:06 AM
  5. Arabic to Roman Numeral
    By jdm71488 in forum C++ Programming
    Replies: 7
    Last Post: 04-21-2004, 01:33 AM

Tags for this Thread


1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21