Hey guys. Just finished one of my favorite algorithm challenges for the JS certificate. YET. Here’s my solution to the problem. What do you think and what would you change?
function convertToRoman(num) {
let romanLetters = ["I", "V", "X", "L", "C", "D", "M"];
let numToStr = num.toString();
let splitNum = [];
let romanNumeral = [];
for(let i = 0; i < numToStr.length; i++) {
splitNum.push(+numToStr.charAt(i));
}
function helperFunc(input, index) {
if (input < 4) {
for(let i = 1; i <= input; i++) {
romanNumeral.unshift(romanLetters[index]);
}
} else if (input === 4) {
romanNumeral.unshift(romanLetters[index] + romanLetters[index + 1]);
} else if (input === 5) {
romanNumeral.unshift(romanLetters[index + 1]);
} else if (input > 5 && input !== 9) {
let plusFive = [romanLetters[index + 1]];
for (let i = 6; i <= input; i++) {
plusFive.push(romanLetters[index]);
}
romanNumeral.unshift(plusFive.join(''));
} else if (input === 9) {
romanNumeral.unshift(romanLetters[index] + romanLetters[index + 2]);
}
else {
romanNumeral.unshift(romanLetters[index]);
}
}
for(let i = splitNum.length - 1; i >= 0; i--) {
if(splitNum[i] == 0) {
continue;
} else {
switch (i) {
case splitNum.length - 1:
helperFunc(splitNum[i], 0);
break;
case splitNum.length - 2:
helperFunc(splitNum[i], 2);
break;
case splitNum.length - 3:
helperFunc(splitNum[i], 4);
break;
case splitNum.length - 4:
helperFunc(splitNum[i], 6);
break;
}
}
}
return romanNumeral.join('');
}
convertToRoman(100);
convertToRoman(1000);
well done solving this one - it’s a challenging one and looks like you nailed it!
The one thing I would recommend is to try to improve the readability of your code a bit. Here are some specific suggestions:
structure your main function so it tells the story of how your code converts to roman numerals top-to-bottom. The helper function should come last in my opinion.
extract the construction of the splitNum into a separate function, so you can give it a name.
give your existing helper function a more meaningful name - what does it actually do?
try to reduce the reliance on referencing things by an array index (e.g. name the individual roman Letters)
learn more about different array methods to make the code more concise - map and reverse in particular are very useful for this challenge.
Yeah I didn’t focus on readability as @camperextraordinaire mentioned as well. Getting it to work was my goal. I should probably move the helper function to the end of the main function and also rename it.
There are a few places I could use map but I can’t see the use of reverse for this one…
Yeah definitely. I did that with the helper function so that it’s not very hard coded. I might need a different approach for numbers greater than 3999.
Referenced that in a previous reply I shall do that.
Hadn’t thought of that. Looks like I have gotten the gist of coding but not the readability of it.
Yeah you’re right. Why don’t you guys add a readability section to the curriculum? I might have missed it if there was… I don’t know. But naming conventions, code structuring and what not would be great. It wasn’t really noticeable to me because it wasn’t that many lines of code and it was a single file.
That’s perfectly fine - you can always revise the code and make it more readable once it’s working. Over time, if you work on it, it will start to come more naturally.
In your code as it is, you have an array splitNum that you loop through from back to front. In each iteration you call a helper function that uses the unshift method on romanNumeral
Both of those are fine, but using reverse you can make it more clear what’s going on. Do you see?
I think this is kind of hard to fit into the way the freecodecamp platform works, but there are some great books on the topic if you want to explore this further - I personally got a lot out of Clean Code, 99 Bottles of OOP, and Refactoring - Improving the Design of Existing Code. Most of the examples in these books are from object oriented languages like Java and Ruby, but the skills they teach transfer really well.
