How do people refactor code?

Hey everyone,

Below is my code for one of the challenges which passes.

function convertToRoman(num) {
  
var decimal = [1, 4, 5, 9, 10, 40, 50, 90, 100, 400, 500, 900, 1000];
var roman = ['I', 'IV', 'V', 'IX', 'X', 'XL', 'L', 'XC', 'C', 'CD', 'D', 'CM', 'M'];
var arr = [];
var value;
  
while(num > 0){
  for(var i = 0; i < decimal.length; i++){
   
   if(num < 0){
      break;
    }else if(num === decimal[i]){
      arr.push(roman[i]);
      num = num - decimal[i];

    }else if(num < decimal[i]){
      console.log(num);
      console.log(i);
      console.log(decimal[i-1])
      arr.push(roman[i-1]);
      num = num - decimal[i-1];
      break;
    }else if(num > 1000){
      arr.push(roman[12]);
      num = num - decimal[12];
    }
    
    if(num === 0){
      break;
    }
  }
}

 value = arr.join("");
 console.log(value);
  
 return value;
}

convertToRoman(1004);

However, I just know it’s not great, there are so many things that could be refactored.

My questions is, how do you go about refactoring code/ improving it.

This code to me just looks crap when I look at some of the other answers online which then discourages you a little for not thinking of it that way.

Cheers
Andy

Like…?

Once you know what needs to be changed, you just start changing things. Don’t get wrapped up in making your algorithm exercises perfect, though. If you’ve finished this, give yourself a well-deserved pat on the back and then move on.

Uh oh!

Well, you usually start and write the code and then try to make it better. Your solution is interesting and it shows that you have thought and understood the problem, Although the algorithm that you use is important but don’t worry about it at this stage, reading the codes written by others is part of the learning. no matter how how you write your code, there’s always a better way, and lots of worst ways. don’t let it discourage you.

So, Putting the algorithm aside, your code has a few problems. some of the ones that got my eyes are these:

var value;

you have declared a variable called “value” with nothing in it. It’s usually unnecessary to do such thing in JS. We will declare it whenever it’s needed.

for(var i = 0; i < decimal.length; i++){
‘decimal’ is an array. although this is usually the way you loop through an array in most languages there is a forEach method in JS that does the job for you. so you probably want go for the JS way.

if(num < 0){
      break;
    }

also

if(num === 0){
      break;
    }

Wait, did you just test if num < 0 and num === 0 in a while loop that only runs if num > 0? how could that ever happen?

while(num > 0){
I would change this with something like this:

if (num <= 0) {
    return;
}
while (true) {

also you have too many console.logs in your code which are supposed to be for testing purposes not something you want to keep.

As I said, There are always better ways to do it but don’t let any of these discourage you. Actually the fact that you know your code can be better and you’re looking for other solutions means that you are learning and improving.

One of the pieces of advice that gets trotted out here fairly often is: Don’t refactor…

…yet.

While you are still learning the fundamentals, just keep chugging through the algorithms.

Each time you solve one, celebrate the win and then move on.

Once you have built some more projects and picked up a few new tricks, you may want to go back and refactor the old algorithms then. Personally, I haven’t bothered with that methodically, but I do hang out here enough to see algorithm questions pop up and when I have a crack at them again just to help others, I find myself naturally reaching for different methods than the ones I used originally.

5 mins after you completed the challenge is the worst time to refactor it, unless you happened to pick up a whole new, paradigm-shifting method in those 5 mins!

Thanks for this guys! Will come back to this solution I think later on and see how I can improve it :slight_smile: