How bad is my solution?

[dka58]

This isn’t exactly a help post, more of a “looking for criticism” kind of post. This is my first topic here. When I first opened the challenge I though to myself that it’s impossible, I wanted to give up right away, but I decided to give it a try anyway. The only copying I did was the table of roman number equivalents (and even that I had to rewrite into key/value pairs). I went on about it through recursion, from the bottom up. Problem is, I hate recursion. I’m a python/c++ developer (junior) and I wanted to go through FCC and because I wanted to get into web development. So I’ve ALWAYS tried to avoid recursion, and JS was no exception. I feel like my code is dirty, messy and just memory wasteful. So any tips and suggestions on how to improve this would be nice.

   **Your code so far**

function convertToRoman(num) {
  let romanNum = {  0: "", 1:"I", 2:"II", 3:"III", 4:"IV" ,5:"V", 6:"VI",7:"VII", 8:"VIII", 9:"IX", 10: "X", 20:"XX", 30:"XXX",	40:"XL", 50:"L", 60:"LX", 70:"LXX", 80:"LXXX", 90:"XC", 100:"C", 200:"CC", 300:"CCC", 400:"CD", 500:"D", 600:"DC", 700:"DCC", 800:"DCCC", 900:"CM", 1000:"M", 2000:"MM", 3000:"MMM", 4000:"MMMM", 5000:"MMMMM", 6000:"MMMMMM", 7000:"MMMMMMM", 8000:"MMMMMMMM", 9000:"MMMMMMMMM"}
  
  let rLen = Object.keys(romanNum).length;
  let resStr = ""; //string that will contain the result
  let flag = false; //flag that stops the recursion once result is achieved
  let leftOverNum = num
  

  if(num > 9000 || num < 0) return "Enter a positive number less than 9000";
  //function that adds to the resulting string the largest roman numeral that
  //is still less than num, untill the remaining number is an exact roman numeral match
  function convertNumLoop(num){
    for(let i = 0; i < rLen; i++){
      if(Object.keys(romanNum)[i] == num){
        resStr += Object.values(romanNum)[i];
        flag = true;
        return resStr;
      }

      if(Object.keys(romanNum)[i] > num){
        resStr += Object.values(romanNum)[i - 1];
        leftOverNum = num - Object.keys(romanNum)[i - 1];
        break;
      }     
    }
  }

  while(!flag) convertNumLoop(leftOverNum);
  
  return resStr;
}

console.log(convertToRoman(1));

Challenge: Roman Numeral Converter

Link to the challenge:

freeCodeCamp.org

[sanity]

Considering you didn’t think you are able to complete it, this is really good enough solution. It works, that’s a main requirement for any code really. Having things working is a good starting point for improvements in the code, whether that’s readability or efficiency.

At first I’d suggest doing some self-reflecting - what when good, what not? Which parts were hard, which were easy? What is sticking the most for you, as for needing improvements, but more specific (for example: instead of make it more memory efficient, try to pin-point where memory is wasted and why) than general thoughts.

Also I think I might have a good news, because I can’t see recursion here.

[JeremyLT]

I agree that it isn’t recursive, but this is not a good challenge to use recursion on.

You can simply the logic somewhat. This is where looking for solutions of others may be helpful, not that you have a working solution.