Roman Numeral Converter , algorithm refactoring

JavaScript
1

is there a way to refactoring this to make it more “efficient”.
or it would be needed to be done with map method to it be better?
sorry i started 2 weeks ago , and im still getting used to the “programmer mindset” (^.^)b
Thanks

Your code so far

function convertToRoman(num) {

let mil = '',valM =[['M'],['MM'],['MMM'],['MMMM']], resM=Math.floor(num/1000);
let cen = '',valC =[['C'],['CC'],['CCC'],['CD'],['D'],['DC'],['DCC'],['DCCC'],['CM']], resC=Math.floor((num/100)-(resM*10));
let dec = '',valD =[['X'],['XX'],['XXX'],['XL'],['L'],['LX'],['LXX'],['LXXX'],['XC']], resD=Math.floor((num/10)-(resC*10)-(resM*100));
let uni = '',valU =[['I'],['II'],['III'],['IV'],['V'],['VI'],['VII'],['VIII'],['IX']], resU=Math.floor((num/1)-(resD*10)-(resC*100)-(resM*1000));
  
  if (resM != 0){ for(let i =0 ; i<resM ;i++) {mil = valM[i] ;} }
  if (resC != 0){ for(let i =0 ; i<resC ;i++) {cen = valC[i] ;} }
  if (resD != 0){ for(let i =0 ; i<resD ;i++) {dec = valD[i] ;} } 
  if (resU != 0){ for(let i =0 ; i<resU ;i++) {uni = valU[i] ;} }
 let result = mil + cen +  dec + uni ;  
 return result;
}

convertToRoman(3999);```
**Your browser information:**

Your Browser User Agent is: ```Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36 OPR/52.0.2871.64```.

**Link to the challenge:**
https://www.freecodecamp.org/challenges/roman-numeral-converter
2

There’s the core of a good solution there - all the bits are in place, but you can cut down the code quite a lot:

  • If you use 1-dimensional arrays rather than an array of arrays it’s easier to select the values - using an array of arrays is unecessary, just make it an array of strings.
  • You don’t need to loop: if you’re calculating the numbers anyway, you can just select from the arrays directly.
  • This leads to you not needing to initialise those variables you use in the loop, you don’t need them anyway - they end up just producing the same value as resC/D/etc
  • If you put an empty string at the start of each array, you don’t need to check for 0.
  • You can, if you want, get rid of the resC, resD etc, and just do valM[Math.floor(n/1000)]

Also, just to make it easier to read and understand, try to keep to a single statement per line, so like

let mil = '';
let valM =[['M'],['MM'],['MMM'],['MMMM']];
let resM=Math.floor(num/1000);

rather than

let mil = '',valM =[['M'],['MM'],['MMM'],['MMMM']], resM=Math.floor(num/1000);
1 Like
3

I tryed to do like you suggested , but i get the problem of when i calculate the next number , like in 3200 , the 200 i used to calculate (3200/100) - (resM*10) // resM would be the result of the previos number calculated, but if i get rid of them i’m passing the string not the numeric value…
any suggestion?
thank you very much for the support , never been in a community and this is quite awesome.

function convertToRoman(num) {

let valueM =[’’,‘M’,‘MM’,‘MMM’,‘MMMM’];
let valueC =[’’,‘C’,‘CC’,‘CCC’,‘CD’,‘D’,‘DC’,‘DCC’,‘DCCC’,‘CM’] ;
let valueD =[’’,‘X’,‘XX’,‘XXX’,‘XL’,‘L’,‘LX’,‘LXX’,‘LXXX’,‘XC’] ;
let valueU =[’’,‘I’,‘II’,‘III’,‘IV’,‘V’,‘VI’,‘VII’,‘VIII’,‘IX’] ;

let mil = valueM[Math.floor(num/1000)];
let cen = valueC[Math.floor( (num/100)-(mil10) )];
let dec = valueD[Math.floor( (num/10)-(cen10)-(mil100) )];
let uni = valueU[Math.floor( (num)-(dec10)-(cen100)-(mil1000) )];

let result = mil + cen + dec + uni ;
return result;
}

4

Ah, sorry, I wasn’t clear there :grimacing:, and with what you have, I’d do:

let mil = Math.floor(num/1000);
...
// then
let result = valueM[mil] + ....

I think that’s the clearest way to do it.

It’s a good solution you had by the way, really easy to refactor

5

sorry , now that i see it , it’s like so obvious , xD
i tend to not touch too much the output variable , and this is very clever to do it that way.

Thank you very much for the support .
it’s been from the 20th of last month i started and i can’t get over the felling of how amazing the community and the study structure is on this, hope i can make my goal of in 3 months be one of the already 500 people that took the 3 certifications .
i’m starting a side project on react and sometimes i just get very mixed ideas xD.
thank you.

1 Like
6

I wrote my code to parse the Roman Numerals like a human would. I won’t post it here, because it seems you want to do the work—an attitude I applaud—but I will tell you where I got the idea for my algorithm. In the lesson, there is a an outlink labelled “Roman Numerals” that points here:
http://www.mathsisfun.com/roman-numerals.html
If you scroll down to the section labelled “Basic Combinations,” you’ll see the numeral pattern laid out in a 3x10 grid. What I noticed is that there is a pattern that repeats once per row that is comprised of two sub-patterns (or four, depending on how you divide them by the || separator below) in A/B, then A/C (using substitute symbols):

A AA AAA || AB ||new subpattern|| B BA BAA BAAA ||BC
C CC CCC ||CD … and so on.

I leave the coding up to you. If you want help fine-tuning the algorithm and nailing down the edge cases, feel free to message back. I recognize that while the code may seem more unwieldy and thus not efficient for the small domain of actual roman numerals (I, V, X, L, C, D, M), it is a more general solution that scales well if we invent symbols for 5 000, 10 000, 50 000 and so on.

(Edit: Fixed the explanation of sub-patterns. It is correct now)