HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Integer to Roman Numeral converter

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
romannumeralintegerconverter

Problem

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
#include

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 && num =500 && 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 <<

Solution

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];
        }
    }
}

Code Snippets

#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];
        }
    }
}

Context

StackExchange Code Review Q#101143, answer score: 4

Revisions (0)

No revisions yet.