What's wrong with it?

[deepanshu895]

Tell us what’s happening:
Describe your issue in detail here.

  **Your code so far**

let arr_num = [];
function thousandBreak(num) {
if (num >= 1000) {
  num -= 1000;
  arr_num.push("M");
} else {
  num -= 900;
  arr_num.push("CM");
}

if (num >= 900) {
  num = thousandBreak(num);
}
return num;
}
function fiveHunBreak(num) {
num -= 500;
if (num >= 0) {
  arr_num.push("D");
} else if (num < 0) {
  arr_num.push("CD");
}
return num;
}
function hunderedBreak(num) {
if (num >= 100) {
  num -= 100;
  arr_num.push("C");
} else {
  num -= 90;
  arr_num.push("XC");
}

if (num >= 90) {
  num = hunderedBreak(num);
}
return num;
}
function fiftyBreak(num) {
num -= 50;
if (num >= 0) {
  arr_num.push("L");
} else if (num < 0) {
  arr_num.push("XL");
}
return num;
}
function tenBreak(num) {
if (num >= 10) {
  num -= 10;
  arr_num.push("X");
} else {
  num -= 9;
  arr_num.push("IX");
}

if (num >= 9) {
  num = tenBreak(num);
}
return num;
}
function fiveBreak(num) {
num -= 5;
if (num >= 0) {
  arr_num.push("V");
} else if (num < 0) {
  arr_num.push("IV");
}
return num;
}

function oneBreak(num) {
num -= 1;
arr_num.push("I");

if (num > 0) {
  num = oneBreak(num);
}
return num;
}

function convertToRoman(num) {
if (num >= 900) {
  num = thousandBreak(num);
} else if (num >= 400) {
  num = fiveHunBreak(num);
} else if (num >= 90) {
  num = hunderedBreak(num);
} else if (num >= 40) {
  num = fiftyBreak(num);
} else if (num >= 9) {
  num = tenBreak(num);
} else if (num >= 4) {
  num = fiveBreak(num);
} else if (num >= 1) {
  num = oneBreak(num);
}

if (num > 0) {
  num = convertToRoman(num);
}
return arr_num.join("");
}

console.log(convertToRoman(1006));

  **Your browser information:**

User Agent is: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.101 Safari/537.36

Challenge: Roman Numeral Converter

Link to the challenge:

freeCodeCamp.org

[ArielLeslie]

@deepanshu895

Tell us what’s happening:
Describe your issue in detail here.

[caryaharper]

To start off you are passing most of the test, but as long as that console.log exist at the bottom of the code they will all fail as your arr_num is global so when you call your convertToRoman function it populates the array causing you to fail all the test, and my recommendation is to not use a global array, but you can simply just git rid of the log and it will most test will pass.

Once you have fixed the console.log problem you will still be failing three test and the reason for this is due to the fact that num can be given to function that will subtract more from it than num has value such as:

else if (num >= 40) {
  num = fiftyBreak(num);

I would generally say this is bad code, but as luck would have it this problem only ever occurs with the fiftyBreak function allowing for a lazy patch to be made, and wether you want to rethink your entire logic or go with the lazy patch is up to you .

The patch works such that if num becomes negative due to the function then you make num equal to the number that would be in its tens place so if num equaled 44 then the function would make num equal to 4 rather than -6. It is important that you only do this as the last thing before returning from the function or simply use a ternary in the return.

[deepanshu895]

FCC doesn’t accept it as solution, when I provide a value in

function convertToRoman(num)

it returns correct Roman Number but when I click on run the test it fails all

[deepanshu895]

Thanks, can you please give me some suggestions why it is a bad code and how I can make it a professional code.

Just some tips

[caryaharper]

I’ll go over a few things I think would help.

An important aspect of your code is how it looks. The harder it is to read you code the less other people are going to want to deal with it so I’ll will start off with some stylistic changes that could be made.

With some exceptions the naming convention in JS is lower camelCase so arr_num should be arrNum.

Indentions and a good amount of white space between things are probably the best thing you can do to make your code more readable. Any time you enter a new inner scope in your code you need to indent inwards and anytime you back out to a outer scope you need to go back to the right indention level:

//global scope no indentions
function  func () {

   //this scope is closed from the global so you should indent
   let a = 1;

   //I put whitespace here to make it more readable 
   if (a) {
      //an (if) is another layer of scope so you should indent
      let b = 2;
     //func can't use b as it doesn’t have access to this inner scope
   }
} 

On the more logical side of things I would say your code is very imperative and this sort of problem certainly leans into a more imperative solution, but if you are to imperative and try to cover every case manually the first moment you function is given a case you didn’t write for it will fail. Generalize where you can and you will save yourself a ton of time and you’ll also have written better code.