I feel very bad about my code(yet it passed the tests) after I saw this basic solution. Ruman Number Converter
Most of the times my code passes the tests but when I see others solutions I think those are better. Please give some suggestions on this code (which I think is very inefficient). I think I make things complex instead of keeping it simple.
**Your code so far**
function convertToRoman(num) {
let romanList = {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' };
if(num >= 4000){
return "Too big number..";
}
if(num <= 0 || num == null){
return "Enter number greater than zero..";
}
let numbersArr = breakNum(num);
let romanNum = [];
for(let i = 0; i < numbersArr.length; i++){
romanNum.push(romanList[numbersArr[i]])
}
return romanNum.join('');
}
// I created this function to break the number so that it will be easier to convert to its equivalent roman number (as shown on this link http://www.mathsisfun.com/roman-numerals.html). For example:- breakNum(399) will return [300, 90, 9].
function breakNum(num){
let rem, newNum;
let numArr = [];
while(rem !== 0){
if(num < 10){
rem = num % num;
newNum = num;
}
if(num >= 10 && num < 100){
rem = num % 10;
newNum = num - rem;
}
if(num >= 100 && num < 1000){
rem = num % 100;
newNum = num - rem;
}
if(num >= 1000){
rem = num % 1000;
newNum = num - rem;
}
numArr.push(newNum);
num = rem;
}
return numArr;
}
convertToRoman(36);
console.log(convertToRoman(3999))
**Your browser information:**
User Agent is: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0
I’d suggest to try to not compare against others. Such comparison rarely is performed fairy by the mind. If you want to compare against somebody, compare yourself against yourself from the past.
Deciding what is better or worse can be a bit subjective, and it can be harder to focus on the specifics, but doing so will help more and gives you more information what to do next. What specifically makes you think your code is worse or ineffective? What did you like in the others? What you don’t like in your version? What do you think should be improved, how that could be done? You’ve mentioned you make things complex, which parts of the code you think are too complex?
Remember that something working is much better than something not working. And having something working is a good point for implementing improvements.
on first read and not going into details i like your solution and i think my own solution, back when i worked on that challenge had to look somewhat similar. I think your approach is on the right track and your doubts are unreasonable. The “basic solution” you give in the link i dont like too much as its too wordy and code seems repetitive, where it could be reduced by a functional approach. The same way you could improve your own solution, where the object with roman numbers reference can be reduced, as there are some values , which can be “generated”, for example 1 holds 1xI, 2 holds 2xI, 10 holds 1xX, 3 holds 3xX, you can see a pattern in the way roman numerals are generated, which can be implemented in the code.
When posting full working solutions make sure to wrap them in [spoiler] tags so it doesn’t spoil it for those that haven’t worked on this problem yet.
Thanks for your reply!!!
Sorry but I’m a little bit confused about the improvement you suggested but as far as I understand the object should look like this : -
romanList ={ 1: 1 x I , 2: 2 x I …,
10: 1 x X, 20: 2 x X…,
};
Earlier I tried to implement like this but I got stuck at 4,40,400 and 9,90,900.
What i mean is, you can work with an object which only holds the unique roman letters; I, V, X, L and so forth. From there on, roman numbers use a patter and with a help of function you can generate the letters. For example: 1- I, 2-II, 3-III; 10-X, 20-XX, 30-XXX. Do you see? You add a zero and only the letter changes, but the patterns remains, it just add the letter for every next number. 100-C, 200-CC, 300-CCC…
5, 6, 7, 8- V, VI, VII, VIII; 50, 60, 70, 80- L, LX, LXX, LXXX(adding a zero, only changed the letters, V=>L, I=>X)
I dont wanna leave the impression in you this is a simple matter and you can right away implement the pattern in a code, im only pointing out in what direction you can go if you were to improve your code, but for now, maybe its a stretch to go that road. For where you are, your code is well written. Improved solutions are written by people with far more experience in coding, which only comes wtih time
Hi, I just finished that same problem and thought my code is repetitive and could be improved so I looked up the forum.
function convertToRoman(num) {
let test = [];
let brokeDownNumber = [];
for (let i = 0; i < num.toString().length; i++) {
if (
Number(num.toString()[i].padEnd(Number(num.toString().length - i), "0")) >
0
) {
brokeDownNumber.push(
Number(num.toString()[i].padEnd(Number(num.toString().length - i), "0"))
);
} else {
brokeDownNumber.push(0);
}
}
for (let i = 0; i < brokeDownNumber.length; i++) {
// Thousands
if (/^[1-9]000/.test(brokeDownNumber[i])) {
test.push("M".repeat(Number(num.toString()[i])));
}
// Hundreds
else if (/^[1-9]00/.test(brokeDownNumber[i])) {
//900
if (num.toString()[i] == 9) {
test.push("CM");
}
//500
else if (num.toString()[i] == 5) {
test.push("D");
}
//400
else if (num.toString()[i] == 4) {
test.push("CD");
}
//800 - 600
else if (num.toString()[i] >= 6) {
test.push(`D${"C".repeat(num.toString()[i] - 5)}`);
}
//300 - 100
else if (num.toString()[i] <= 3) {
test.push("C".repeat(num.toString()[i]));
}
}
//Tens
else if (/^[1-9]0/.test(brokeDownNumber[i])) {
//90
if (num.toString()[i] == 9) {
test.push("XC");
}
//50
else if (num.toString()[i] == 5) {
test.push("L");
}
//40
else if (num.toString()[i] == 4) {
test.push("XL");
}
//8-6
else if (num.toString()[i] >= 6) {
test.push(`L${"X".repeat(num.toString()[i] - 5)}`);
}
//30 - 10
else if (num.toString()[i] <= 3) {
test.push("X".repeat(num.toString()[i]));
}
}
//Numbers
else if (/[1-9]/.test(brokeDownNumber[i])) {
//9
if (num.toString()[i] == 9) {
test.push("IX");
}
//5
else if (num.toString()[i] == 5) {
test.push("V");
}
//4
else if (num.toString()[i] == 4) {
test.push("IV");
}
//8-6
else if (num.toString()[i] >= 6) {
test.push(`V${"I".repeat(num.toString()[i] - 5)}`);
}
//3-1
else if (num.toString()[i] <= 3) {
test.push("I".repeat(num.toString()[i]));
}
}
}
let result = "";
test.forEach((el) => (result += el));
return result;
}
console.log(convertToRoman(1023));
This is my code here, we started similarly but I choose an easier way to get the test done and repeated myself 3 times for thousands, hundreds, tens, and numbers .
Imo, as we are learning here worrying about those details, is unnecessary.
I would suggest you that save your code somewhere and after few months come back and solve the same problem for comparison. This will not just show how much you improved and also can be quite motivational.
Regex is a pretty heavy tool that should only be used where needed.
In both of these solutions, there is a lot of repeated logic. It is good that you each identified the core algorithm to solve the problem, but how can you distill this logic down to avoid all of this repetition?
Sorry I didn’t understand that earlier. As @Russell-Ali, @sanity and you stated that,
Improvement = Experience;
Experience = Time;
"Improvement in the code comes with experience and experience in coding comes with time. "
I’ll try to find a more better soution very soon. Thanks again.
That’s pretty common for most people. Don’t feel bad about that.
I’m just a beginner but what I do, in order to be more efficient, is drawing the algorithm you’re going to write. In other words, you draw the flowchart before writing the code. I usually use https://app.diagrams.net/. That way I can figure out more easily if I’m going into unnecessary processes.
.