1
\$\begingroup\$

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);
        Console.ReadLine();
\$\endgroup\$
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ – Jesse C. Slicer Oct 22 '18 at 0:24
  • \$\begingroup\$ digitFactorial can be removed too, can't it? And work with 'number' all along. Instead of the for, I'd make a recursive method \$\endgroup\$ – D Ie Oct 22 '18 at 6:34
  • \$\begingroup\$ Incorrect functionality when number == 0. ref \$\endgroup\$ – chux Oct 22 '18 at 8:14
3
\$\begingroup\$

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;
    }
  }
\$\endgroup\$
  • \$\begingroup\$ 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; }? \$\endgroup\$ – chux Oct 22 '18 at 8:28
1
\$\begingroup\$

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;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.