Comment on this Factorial Class

[mpark4656]

I am fairly new to C++ Programming and I'd like some input on this program.

This is a simple program that calculates Factorial of a given integer.

With this program, I attempted to demonstrate the following C++ Concepts:
1. Multi-File
2. Class
3. User-Defined Constructor
4. Recursion
5. << Operator Overloading / friend function
6. Exception Handling

There is a total of 3 files:
1. .cpp that contains the main function
2. .cpp file that contains implementation
3. .h file that contains class definition

Here are the codes.


main.cpp

#include <iostream>
#include "factorial.h"

int main ()
{
    long long number;
    bool passed = false;

    do
    {
        try
        {
            std::cout << "Enter the number (Accurate up to 20): ";
            std::cin >> number;
            std::cout << std::endl;

            if ( number < 0 )
                throw number;
            if ( !std::cin )
                throw number;

            passed = true;          // If no exception found, end the while-loop
        }
        catch ( long long x )
        {
            std::cout << "Non-Positive or Invalid Input: " << x << std::endl;
            std::cout << "Restoring the Input Stream...\n\n";
            std::cin.clear ();
            std::cin.ignore ( 100 , '\n' );
        }
    }
    while ( !passed );

    Factorial factorial ( number );

    std::cout << factorial;

    return 0;
}

factorial.h

#ifndef FACTORIAL_H_INCLUDED
#define FACTORIAL_H_INCLUDED

#include <iostream>

class Factorial
{
    long long factorial;
    long long number;           // Store Original Input

    // Demonstrates Direct Recursion
    long long calculateFactorial( long long number );

public:
    // Default Constructor
    Factorial ( long long num );

    // Demonstrates << OverLoading
    friend std::ostream& operator<< ( std::ostream& os , Factorial factorial );
};

#endif // FACTORIAL_H_INCLUDED 

factorial.cpp

#include <iostream>
#include "factorial.h"

// Constructor that Requires long long type variable
Factorial::Factorial ( long long num )
{
    number = num;
    factorial = calculateFactorial ( number );
}

// Member Function that Utilizes Recursion to calculate Factorial
long long Factorial::calculateFactorial( long long number )
{
    if ( number == 0 )
        return 1;
    else
        return number * calculateFactorial ( number - 1 );
}

// Overloaded << Operator that returns the calculated factorial to Standard Output
std::ostream& operator<< ( std::ostream& os , Factorial factorial )
{
    os << "The Factorial of " << factorial.number << " is " << factorial.factorial << std::endl;

    return os;
}

I'd like some feedback on code readability, use of all the C++ concepts, and possibly any tips on how I can get this program to calculate the factorial of a number higher than 20.

Thanks In Advance!

[booradley60]

Why maintain any state at all? Calculating factorial is a function with distinct input and output, no state needed. If you want to cache some sort of lookup for future use, you could just use an array (or vector) since you can't calculate factorials for inputs higher than 20 anyway:

#include <iostream>
#include <vector>
#include <cstdint>

std::uint64_t calculateFactorial(std::uint8_t num)
{
    if (num > 20) throw num;

    if (num == 0)
    {
        return 1;
    }
    else
    {
        return num * calculateFactorial(num - 1);
    }
}

int main()
{
    std::vector<std::uint64_t> factorialLookup;
    for (int i = 0; i <= 20; ++i)
    {
        factorialLookup.push_back(calculateFactorial(i));
    }

    for (int i = 0; i <= 20; ++i)
    {
        std::cout << i << "! equals " << factorialLookup[i] << std::endl;
    }
    return 0;
}

[mpark4656]

@booradley60

I'm sorry. I don't understand what you mean by "maintaining any state". Are you saying that it does not make any sense to ask for input from user when you can just print out all 20 factorials?

Anyways, you are right. It's just 20 and I could just print out all 20 factorials for lookup.

[cire]

I'm sorry. I don't understand what you mean by "maintaining any state".

You have more complexity than you need. And, although in this case the complexity hasn't reached an intolerable level, in general you want to avoid complexity that doesn't buy you anything. It makes things harder to reason about and introduces entry points for possible errors (perhaps as evidenced by the following points.)

You store two values in your Factorial class. The number object is only used as a copy of a local variable in the constructor (in the other member function it isn't even used,) so it can be done without. The factorial member is only used to store a value. That value is never used, otherwise, so that can also be done without.

Your constructor performs work which is never utilized. The same work must be done when calculateFactorial is called on a class object and there is no other way to retrieve that cached value.