8
\$\begingroup\$

I have implemented an Integer to Roman Numeral converter that converts any number in the range \$[1 \le x \le4999]\$ to its Roman number equivalent. Any comments on improving my code style or organization?

#include <string>
#include <iostream>

class Solution{
public:
    Solution () : IntVal {1,5,10,50,100,500,1000} , roman {'I', 'V' , 'X', 'L' ,'C', 'D' ,'M'} {}
    std::string intToRoman(int num);
private:
    int IntVal[7];
    char roman[7];
    int fslt(int num);
    std::string output;
    void helper(int num);
    void postProcess();
    void str_replace( std::string &s, const std::string &search, const std::string &replace);
};


#include "roman.hpp"

int Solution::fslt(int num){
    if (num>=50){
        if (num<100) return 3;
        else if (num>=100 && num < 500) return 4;
        else if (num>=500 && num < 1000) return 5;
        else return 6;
    }
    else{ // num < 50
        if (num>=10) return 2;
        else if (num>=5) return 1;
        else return 0;
    }
}

void Solution::helper (int num){
    int div = fslt(num);


    if (div == 0){
        for (int i = 0; i <num ;i++){
            output += roman[div];
        }
         return;
    }
    else {
        int rn = fslt(num);
        int numTimes = num/IntVal[rn];
        for (int i=0;i < numTimes; i++){
            output += roman[rn];
        }
        helper(num-numTimes*IntVal[rn]);
    }
}

void Solution::postProcess(){
    str_replace(output, "DCCCC", "CM");
    str_replace(output, "CCCC", "CD");
    str_replace(output, "LXXXX", "XC");
    str_replace(output, "XXXX", "XL");

    str_replace(output, "VIIII", "IX");
    str_replace(output, "IIII", "IV");
}

std::string Solution::intToRoman(int num){
    Solution::helper(num);
    Solution::postProcess();
    return output;
}

void Solution::str_replace( std::string &s, const std::string &search, const std::string &replace)
{
     for( size_t pos = 0; ; pos += replace.length() ) 
    {
         pos = s.find( search, pos );
         if( pos == std::string::npos ) break;

         s.erase( pos, search.length() );
         s.insert( pos, replace );
     }
}

int main(){
    Solution mySol;
    std::cout <<  mySol.intToRoman(4999) << std::endl;
    return 0;

}
\$\endgroup\$
2
  • \$\begingroup\$ Using three lookup tables, one for each of the last three digit would be much simpler. \$\endgroup\$ Commented Aug 17, 2015 at 8:30
  • \$\begingroup\$ here is one written for code golf. codegolf.stackexchange.com/a/828/186 \$\endgroup\$ Commented Aug 17, 2015 at 21:53

1 Answer 1

4
\$\begingroup\$

Helper function too complex

Your helper function and the fslt function seem too complex for what they do (helper is even recursive!). All you are really doing is adding roman numerals starting at the highest value first. So something like this would suffice:

#define DIM(arr)        (sizeof(arr)/sizeof(arr[0]))

void Solution::helper (int num){
    for (int i = DIM(IntVal)-1; i >= 0; i--) {
        while (num >= IntVal[i]) {
            output += roman[i];
            num -= IntVal[i];
        }
    }
}
\$\endgroup\$

Your Answer

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

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