# Generate the Factorial of Given Number

This is a console application that generates the factorial of a given number. Is there any way I can improve my code ?

        int number = Convert.ToInt32(Console.ReadLine());
int digitFactorial = number;
for (int counter = number - 1; counter > 0; counter--)
{
digitFactorial *= counter;
}
Console.WriteLine(digitFactorial);

• First, try extracting the factorial calculation itself into its own method and call that method from your interactive part of the program. That's a pretty decent exercise in itself for learning stepwise refinement. Commented Oct 22, 2018 at 0:24
• digitFactorial can be removed too, can't it? And work with 'number' all along. Instead of the for, I'd make a recursive method
– D Ie
Commented Oct 22, 2018 at 6:34
• Incorrect functionality when number == 0. ref Commented Oct 22, 2018 at 8:14

As Slicer wrote in his comment you should create a method/function for the algorithm. And you could make it as an extension method:

public static class NumberExtensions
{
public static ulong Factorial(this ulong number)
{
if (number < 2) return 1;

checked
{
ulong factorial = number;
for (ulong i = 2; i < number; i++)
{
factorial *= i;
}
return factorial;
}
}
}


Here I've widened the domain to ulong and I iterate upwards from 2 as it is more intuitive and easier to read. Further I've encapsulated the method in a checked statement in order to throw on overflow, because factorial grows quickly out of even the scope of ulong.

Usage

  while (true)
{
Console.Write($"Enter a number: "); if (ulong.TryParse(Console.ReadLine(), out ulong number)) { try { Console.WriteLine($"Factorial of {number} = {number.Factorial()}");
}
catch (OverflowException ex)
{
Console.WriteLine($"Entered number is too big: {number}"); } catch (Exception ex) { Console.WriteLine($"An Error occured: {ex.Message}");
}
}
else
{
break;
}
}

• Instead of a special if (number < 2) return 1; test, maybe only a loop: ulong factorial = 1; for (ulong i = 2; i <= number; i++) { factorial *= i; }? Commented Oct 22, 2018 at 8:28

Quoting Wikipedia:

In mathematics, the factorial of a non-negative integer n, denoted by n!, is the product of all positive integers less than or equal to n.

So, you can use an 'unsigned' to don't have to deal with negative number.

Taking a 'ulong' you can calculate factorial until 20 (instead of 12 with a 'int' in your implementation). If you want a larger range, try returning a 'decimal' or deal with 'BigInteger' from 'System.Numerics'.

The value of 0! is 1, according to the convention for an empty product.

You need catching this special case (what you dont), and set returning value according.

Now, we have to make the product of all numbers less than N:

n! = 1 * 2 * 3 * ... * (n-2) * (n-1) * n

We can reverse order, multiplication being commutative:

n! = n * (n-1) * (n-2) * ... * 3 * 2 * 1

or since *n * 1 = n*, we can reduce:

n! = n * (n-1) * (n-2) * ... * 3 * 2

or another notation:

n! = n * (n-1) * (n-2) * ... * (n-n+2)

Resulting to:

static ulong factorial(ulong n)
{
ulong result = (n > 0) ? n : 1;
while (n > 1) result *= n--;
return result;
}