Why does my factorial number finder return the number inputted in C++? (edited)

Question

I have written a factorial number finder, unforutunately it returns the number that is inputted. Any help is appreciated.

#include <iostream>
#include <string>

using namespace std;

int main()
{
int factorialNumber;
int factorialNumber2;
int counter = 1;



cout << "Enter a number and I shall find its factorial value.";
cin >> factorialNumber2;
factorialNumber=factorialNumber2;

while(counter == factorialNumber2);
{  
    factorialNumber=counter * factorialNumber;
    counter++;
}

cout << factorialNumber;

}

You collect user input into variable `factorialNumber2` - then promptly overwrite it with random uninitialized garbage (stored in `factorialNumber`) on the very next line. — Igor Tandetnik, Jul 14 '17 at 03:38
Please do not correct your code in the post. That makes answers seem wrong. — klutt, Jul 14 '17 at 04:09

klutt · Accepted Answer · 2017-07-14T04:05:48.223

There's some problems here. First, you reset the value of factorialNumber2 to something undefined.

Next problem is that you have a semicolon after the while loop and that the condition for the loop was wrong. Here is working code:

int main()
{
    int number;
    int factorialNumber;
    int counter = 1;

    cout << "Enter a number and I shall find its factorial value.";
    cin >> number;
    factorialNumber=1;

    while(counter <= number)
    {  
        factorialNumber=counter * factorialNumber;
        counter++;
    }

    cout << factorialNumber;
}

I renamed the variables, cause factorialNumber and factorialNumber2 didn't really make sense. Another thing you should think of is to not use using namespace. It will hardly affect you at your level, but it's generally a bad idea. Read why here

Also, a pretty good way to start debugging this would have been just some printouts. Here is an example:

int main()
{
    int number;
    int factorialNumber;
    int counter = 1;

    cout << "Enter a number and I shall find its factorial value.";
    cin >> number;
    factorialNumber=1;

    cout << "Before loop" << endl
         << "number: " << number 
         << " factorialNumber: " << factorialNumber
         << " counter: " << counter << endl;

    while(counter <= number)
    {  
    cout << "Begin loop" << endl
         << "number: " << number 
         << " factorialNumber: " << factorialNumber
         << " counter: " << counter << endl;


        factorialNumber=counter * factorialNumber;
        counter++;

    cout << "End loop" << endl
         << "number: " << number 
         << " factorialNumber: " << factorialNumber
         << " counter: " << counter << endl;
    }

    cout << "After loop" << endl
         << "number: " << number 
         << " factorialNumber: " << factorialNumber
         << " counter: " << counter << endl;

    cout << factorialNumber;
}

It looks a bit crappy, but those printouts are intended to be temporary anyway. Also, of course this is a bit overkill, but it shows the general idea.

Here is what it would have looked like with your original code:

$ ./a.out 
Enter a number and I shall find its factorial value.5
Before loop
number: -155928352 factorialNumber: -155928352 counter: 1
Begin loop
number: -155928352 factorialNumber: -155928352 counter: 1
End loop
number: -155928352 factorialNumber: -155928352 counter: 2
After loop
number: -155928352 factorialNumber: -155928352 counter: 2
-155928352

With a little experience, you can clearly see what's wrong here. Lastly, use compiler flags to get warnings. Here is an example:

$ g++ yourcode.cpp -Wall -Wextra
yourcode.cpp: In function ‘int main()’:
yourcode.cpp:18:3: warning: this ‘while’ clause does not guard... [-Wmisleading-indentation]
   while(counter == factorialNumber2);
   ^~~~~
yourcode.cpp:19:3: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘while’
   {
   ^

Saranjith · Answer 2 · 2017-07-14T04:31:58.660

In your code remove semicolon after while loop condition statement

and replace

factorialNumber=factorialNumber2;

with

factorialNumber=1;

Also chnage while loop condition to

while(counter <= factorialNumber2)

Because this is the variable you're using to store factorials. SO firstly value should be 1.

Whole Code will become

#include <iostream>
#include <string>

using namespace std;

int main()
{
int factorialNumber;
int factorialNumber2;
int counter = 1;

 factorialNumber2=5;
factorialNumber=1;

while(counter <= factorialNumber2)
{  
    factorialNumber=counter * factorialNumber;
    counter++;
}

cout <<"Value is"<< factorialNumber;

}

The Most efficient way of find out factorial is using recursive function.

#include <iostream>
#include <string>

using namespace std;
float fact;

int main()
{
  int inputNumber;

  cout << "Enter a number and I shall find its factorial value.";
  cin >> inputNumber;
  float result=factorial(inoutNumber);
  cout << result; 
}

float factorial(n) {

  fact = 1;   
  for i = 1 to N do     
  fact = fact*i; 
  return fact;
}

Salman · Answer 3 · 2017-07-14T04:59:20.487

Your while condition is counter==factorialNumber2" and its mean that it will run only when both variable values are equal. So while condition always false because couneter start from 1. That's why your program return the value you entered . so try to use following code.

 #include <iostream>
    #include <string>
    using namespace std;
    int main()
    {
    int factorialNumber;
    int factorialNumber2;
    int counter = 1;
    cout << "Enter a number and I shall find its factorial value.";
    cin >> factorialNumber2;
    factorialNumber= factorialNumber2;

    while(counter <= factorialNumber2);
    {  
        factorialNumber=counter * factorialNumber;
        counter++;
    }

    cout << factorialNumber;

    }

Why does my factorial number finder return the number inputted in C++? (edited)

3 Answers3