30-seconds-of-code: Factorial of negative numbers

Created on 15 Dec 2017  路  10Comments  路  Source: 30-seconds/30-seconds-of-code

Why is factorial snippet handling negative numbers? I do not think factorial for negative numbers is defined.

Snippet from factorial.md :
const factorial = n => n <= 1 ? 1 : n * factorial(n - 1);

enhancement good first issue help wanted

Most helpful comment

The factorial of 0 is actually 1. I've been taught this multiple times and a quick google search says so. So technically factorial(0) -> 1 is correct. Safeguards for negative numbers are not in place, but I expect we can easily PR that to fix the problem.

All 10 comments

the code returns 1 for any value lower than 1 and the recursion ends, which actually mean factorial(0) returns 1, which is wrong as it should return 0.

negative numbers are an invalid input and none of these example functions really have any guards or validation, so that probably isn't 'wrong' however !0 === 1 is mathematically incorrect

The factorial of 0 is actually 1. I've been taught this multiple times and a quick google search says so. So technically factorial(0) -> 1 is correct. Safeguards for negative numbers are not in place, but I expect we can easily PR that to fix the problem.

Similar safeguards can be added in multiple other examples. For example, x modulo 0 is not defined in pratice (snippet does not handle this!)

this will be helpful @monikadhok @Chalarangelo

let factorial = n => n < 0 ? 
(() => { throw new TypeError('Negative numbers are not allowed!') })() : n <= 1 ? 
1 : n * factorial(n - 1);

We are currently discussing, under #172, the need for error handling. In general, we're probably going to go with no error handling except if it's necessary. In the factorial snippet, I think it isn't, but I am a little opinionated on that. More opinions?

Factorials of negative numbers are dependent on if it's even or odd, and by definition are -Infinity or Infinity because there is no way for the construct( algorithm ) to stop( it is defined stopped at special case 0/-0 )

Ideally it should be

const factorial = n => n < 0 ? n % 2 == 0 ? Infinity : -Infinity : n == 0 ? 1 : n * factorial(n-1)

@skatcat31 That sounds like a good way to handle this, let's get it fixed then.

@skatcat31 It should be const factorial = n => n < 0 ? Math.abs(n) % 2 == 0 ? Infinity : -Infinity : n == 0 ? 1 : n * factorial(n-1)
Javascript gives -8%2 == -0

@kriadmin -0 === 0 === true, lets us avoid a function call


Where our snippet fix fail is in handling decimals since it is supposed to clamp them to the nearest integer value

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for any follow-up tasks.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

henrycjchen picture henrycjchen  路  4Comments

skatcat31 picture skatcat31  路  5Comments

fplgusmao picture fplgusmao  路  4Comments

Bruce-Feldman picture Bruce-Feldman  路  4Comments

Priyansh2001here picture Priyansh2001here  路  5Comments