Factorialize a Number - Which Solution is Better and Why?

Algorithm Challenge. Just wondering the pros and cons of these two solutions and which if either is better and why.

I came up with this solution.

function factorialize(num) {
  var factode = 1;
  for(var i = num; i > 0; i--) {
    factode *= i;
    
  }
  return factode;
}
  

factorialize(5);

but the wiki provides this solution…

function factorialize(num) {
  if (num === 0) { return 1; }
  return num * factorialize(num-1);
}

factorialize(5);

Better not to use recursive functions if possible.

The recursive solution (from the wiki) might be less performant for large num, although this depends on the JS compiler optimizing the code. Research JS recursion optimization to find out more and profile it to see if there even is a performance hit and if so what magnitude it is.

The downside of the imperative solution (yours) is that programming in this style is sensitive to off-by-one errors in the for statement, which can be hard to detect. Manually creating variables like factode could, in a more complicated function, potentially leak out.

The upside of the imperative solution is mostly (maybe) performance, and to a lesser extent an inexperienced programmer might have an easier time understanding it.

The upside of the recursive solution is readability and simplicity. It’s should immediately be obvious what the function does, and it naturally follows from the definition of what a factorial is. There are no local variables, so there is no possibility of any variable values leaking out.

So for the most part I would suggest this comes down to performance vs readability. Make your choice.

Since (this kind of difference in) performance doesn’t generally matter that much , I’m not concerned there. Plus, if you later discover that this specific function is slowing down the performance of the entire application, you can optimize then anyway.

Code is read much much more often than it is written, so I prioritizing simple readable code is a great choice, IMO.

Also, just to save one cycle, your for loop only needs to go to i > 1 - no need to multiply by one. :wink:

Just finished the challenge, this is my solution and it’s working.
Is anyone knows if it’s correct? I mean, I know it is because it works, but do you think it’s a good solution?

function factorialize(num) {
  if (num === 0) {
    return 1;
  }
  
  var n = num;
  for (i = 1; i <= n; i++) {
    num *= i;
  }
  return num / n;
}

factorialize(5);

You have an off by one error that return num/n; fixes to give the right answer. If you input 2, you get num=2 and n=2 before your for loop. First iteration num=2 second iteration num=4 then returning num/n returns 4/2 giving the correct answer, but multiplying by the input twice then dividing by the input, rather than simply mutiplying by it only once (for 3 it would return 18/3, 4 would give 96/4, etc). This obfuscates what the function is actually trying to do. A better solution would be adjusting your loop or variables to eliminate the error and then simply return num;. Hope that actually makes sense and helps!

See above discussion for a solution using a loop vs a solution using recursion. Your solution (once the error I mention is fixed) is equivalent to the solution the OP came up with (as opposed to the wiki solution he quotes).

I agree with medicliffy but again point out the silliness of starting the loop with i=1 - there is no need to multiply times 1. Start with i=2. And like he said, rearrange your loop so you donlt have to divide.

If you do that, also remember to make num=1 and explicit case in the first “if” else factroialize(1) will create an infinite loop. But agreed, multiplying by 1 is unnecessary.
Edit as corrected by @ksjazzguitar, it would never enter the loop and just return 1.

No, as the code stands now (referring to lbluigi’s code), it would return 1. It would not enter the loop (i would be greater than num) so it would skip the loop and return 1/1.

But the biggest thing is to get rid of that division at the end. lbluigi, let me put it this way. You’re setting n=num and then at the end of you’re loop you’re multiplying times n again. You’re basically multiplying times n twice which is why you have to cancel it out with that division at the end. The solution is either to initialize num=1 or to exit the loop before i=n. But in either case, the first step of the loop, multiplying by 1 is a waste.

True that. I didn’t look closely enough.