What is the issue with this factory function?

Question

In the below code, I write a factory method to create objects of a type in a class heirachy.

#include <iostream>
#include <memory>
using namespace std;

enum Type {
    _Base, _A, _B, _C
};

class Base{
private:
    Type type = _Base;
public:
    virtual Type getType(){
        return type;
    }};

class A : public Base{
private:
    Type type = _A;
public:
    using Base::Base;
};

class B : public Base{
private:
    Type type = _B;
public:
    using Base::Base;
};

class C : public Base{
private:
    Type type = _C;
public:
    using Base::Base;
};

shared_ptr<Base> letterFactory(Type which){
    shared_ptr<Base> base = make_unique<Base>(Base());
    switch (which){
        case _A:
            base = make_unique<Base>(A());
        case _B:
            base = make_unique<Base>(A());
        case _C:
            base = make_unique<Base>(C());
    }
    return base;
}


int main(){
    shared_ptr<Base> instanceOfA = letterFactory(_A);
    cout << instanceOfA->getType() << endl;

    shared_ptr<Base> instanceOfB = letterFactory(_B);
    cout << instanceOfB->getType() << endl;

    shared_ptr<Base> instanceOfC = letterFactory(_C);
    cout << instanceOfC->getType() << endl;

    return 0;
};

The output is

0
0
0

How can I make the output

1
2
3

Answer 1

I refactored your code a bit just to apply some of the best practices:

• use enum class instead of the raw enum because enum class represents a scoped enumeration type and it is also strongly typed which means you cannot convert it that easily to integer as the raw enum (that's why we have to_integral template function)
• prefer not to use using namespace std; because it imports the entirety of the std namespace into the current namespace of the program
• also, I think you wanted to use std::make_shared instead of std::make_unique

#include <memory>
#include <iostream>

enum class Type : int {
    _Base = 0,
    _A    = 1,
    _B    = 2,
    _C    = 3
};

class Base{
private:
    Type type = Type::_Base;
public:
    virtual Type getType(){
        return type;
    }
};

class A : public Base{
private:
    Type type = Type::_A;
public:
    virtual Type getType() override {
        return type;
    }
};

class B : public Base{
private:
    Type type = Type::_B;
public:
    virtual Type getType() override {
        return type;
    }
};

class C : public Base{
private:
    Type type = Type::_C;
public:
    virtual Type getType() override {
        return type;
    }
};

std::shared_ptr<Base> letterFactory(Type which){
    switch (which){
        case Type::_A:
            return std::make_shared<A>();
        case Type::_B:
            return std::make_shared<B>();
        case Type::_C:
            return std::make_shared<C>();
        default:
            return std::make_shared<Base>(Base());
    }
}

template <typename Enum>
constexpr typename std::enable_if<std::is_enum<Enum>::value,
                                  typename std::underlying_type<Enum>::type>::type
to_integral(Enum const& value) {
    return static_cast<typename std::underlying_type<Enum>::type>(value);
}

int main(){
    std::shared_ptr<Base> instanceOfA = letterFactory(Type::_A);
    std::cout << to_integral(instanceOfA->getType()) << std::endl;

    std::shared_ptr<Base> instanceOfB = letterFactory(Type::_B);
    std::cout << to_integral(instanceOfB->getType()) << std::endl;

    std::shared_ptr<Base> instanceOfC = letterFactory(Type::_C);
    std::cout << to_integral(instanceOfC->getType()) << std::endl;

    return 0;
};

Live example

Answer 2

Your factory is a bit flawed and your getType() function is not overridden in the derived classes. I guess you wanted to do something along this lines:

#include <iostream>
#include <memory>

using namespace std;

enum Type {
    _Base, _A, _B, _C
};

class Base{
public:

    virtual ~Base() = default;
    virtual Type getType() const {
        return _Base;
    };
};

class A : public Base{
public:
    virtual Type getType() const override {
        return _A;
    };
};

class B : public Base{
public:
    virtual Type getType() const override {
        return _B;
    };
};

class C : public Base{
public:
    virtual Type getType() const override {
        return _C;
    };
};

unique_ptr<Base> letterFactory(Type which){
    switch (which){
        case _Base:
            return make_unique<Base>();
        case _A:
            return make_unique<A>();
        case _B:
            return make_unique<B>();
        case _C:
            return make_unique<C>();
    }

    return nullptr;
}


int main(){
    shared_ptr<Base> instanceOfA = letterFactory(_A);
    cout << instanceOfA->getType() << endl;

    shared_ptr<Base> instanceOfB = letterFactory(_B);
    cout << instanceOfB->getType() << endl;

    shared_ptr<Base> instanceOfC = letterFactory(_C);
    cout << instanceOfC->getType() << endl;

    return 0;
};

Notice that we got rid of the type member completely and instead properly overrode the getType() function. Furthermore, factory functions like this usually return unique_ptr (which can be implicitly converted to shared_ptr if you really want to).

Answer 3

Your Base class has a member type and a virtual member function getType() that returns the value of the member type. Your classes A, B, and C derive from Base. That means they all have a Base subobject. That subobject contains the member Base::type. In addition, they all also add another member type that is then not ever used by anything. Also, none of them override the getType method either. So whenever you call

instanceOfX->getType()

even if instanceOfX points to an instance of one of the derived classes, since none of the derived classes overrite getType, you'll end up calling Base::getType, which will return the value of Base::type, which is always _Base…

What you actually wanted was probably something along the lines of:

struct Base
{
    virtual Type getType() const = 0;

protected:
    Base() = default;
    Base(Base&&) = default;
    Base(const Base&) = default;
    Base& operator =(Base&&) = default;
    Base& operator =(const Base&) = default;
    ~Base() = default;
};

class A : public Base
{
public:
    Type getType() const override { return _A; }
};

class B : public Base
{
public:
    Type getType() const override { return _B; }
};

class C : public Base
{
public:
    Type getType() const override { return _C; }
};

Note that this is almost certainly bad design. The only purpose such a getType method could serve is so that client code can find out the concrete type of the object that a Base* it got is pointing to. If you ever need this information, your design violates the Liskov Substitution principle…

Apart from all that, note that _Base, _A, _B, and _C are reserved names [lex.name]/3 that you're not supposed to use in C++ code…