Roman Numeral Solution Feedback

Code Feedback
1

Hey guys, I’m wondering if anyone could give me some feedback on my Roman Numeral Solution. On one hand I quite like what I’ve done as there is recursion and emphasis on functional programming so I D.R.Y. But after completing the project and looking at the solutions suggested I’m wondering if I overdid it? How would this fly in a dev environment? Thanks :slight_smile:

function convertToRoman(num) {

// Divide num by 1000 to get a base figure to derive number of thousands in num. 
// Will then be used to derive number of hundreds, tens and singles that make num.

  let baseNum = (num/1000).toFixed(3);


  function integerCheck(base) {
    let checker = Math.floor(base);

    return (base - checker) * 10;
  
  }

  let thousands = Math.floor(baseNum);
  let hundreds = Math.floor(integerCheck(baseNum));
  let tens = Math.floor(integerCheck(integerCheck(baseNum)))
  let ones = Math.round(integerCheck(integerCheck(integerCheck(baseNum))));
  

  let romanArr = [];

  function pushRomanDigit(arr, num, singleNum, midNum, maxNum) {

  	if (singleNum == "M") {
  		for (let i = 0; i < num; i++) {
  			romanArr.unshift("M");
  		}
  	} else {
  		if (num >= 5) {
	  		if (num == 9) {
	  			arr.unshift(maxNum);
	  			arr.unshift(singleNum);

	  		} else {
	  			let pushI = num - 5;

				for (let j = 1; j <= pushI; j++) {
					arr.unshift(singleNum);
				}
			  	arr.unshift(midNum);
	  		}
	  	
	  } else {
	  	if (num <= 4) {
	  		if (num == 4) {
	  			arr.unshift(midNum);
	  			arr.unshift(singleNum);
	  			
	  		} else {
	  			for (let j = 1; j <= num; j++) {
	  				arr.unshift(singleNum);
	  			}
	  		}
		}
	  }
  	}

  	

  return arr;
  }

  pushRomanDigit(romanArr, ones, "I", "V", "X");
  pushRomanDigit(romanArr, tens, "X", "L", "C");
  pushRomanDigit(romanArr, hundreds, "C", "D", "M");
  pushRomanDigit(romanArr, thousands, "M", "", "");

  

  console.log(romanArr);



 return romanArr.join("");
}

convertToRoman(4234)
2

It’s quite complicated, and has lots of stuff going on – nested conditional statements in particular are extremely difficult to follow, as is use of push/unshift (vs. methods that do not mutate).

It would make your life a lot easier I think if you started with an reversed array of the digits of the input number, so like input is 1234, but you start with [4, 3, 2, 1]. Then you can convert to I’s or V for the first element, X’s or L’s or whatever for the second, and so on and so forth. You don’t need most of the conditional stuff then, as you can just loop over the array of digits (then you finally reverse back and join once that’s done).

There are multiple ways to get that array, for example like

/**
 * Given an integer, convert to an array of single digits in the reverse order.
 * Works by converting the number to a string, splitting it down into
 * individual charaters, reversing it, then converting each character to an
 * integer.
 *
 * @param {number} n
 */
function reversedDigits (n) {
  return n.toString().split('').reverse().map(d => parseInt(d, 10));
}

Or recursively, without stringifying:

/**
 * Given a number, repeatedly appends the last digit
 * to the output array before rerunning the function,
 * passing in the integer part of the number divided by 10.
 * Stops recursion once number reaches 0, at which point
 * the output array is returned.
 *
 * @param {number}   n      - an integer
 * @param {number[]} digits - an array of digits, populated by
 *                            and eventually returned by the function
 */
function reversedDigits (n, digits = []) {
  if (n === 0) return digits;
  return reversedDigits(Math.trunc(n / 10), digits.concat(n % 10));
}
1 Like
3

Hey thanks for responding so quickly, I’m going to have another crack at this once I complete a few of the projects and attempt to create something more reader friendly and concise as I see what you’re saying and why iterating throuhg the digits in a reversed order would create a more functional program.

thanks again!