C++ Class Design and return by value

Question

I have a class hierarchy that looks like this:

class Address {
public:
    virtual ~Address() {}

    enum Type {
        Internal, External
    };

    Type type() const { return _type; }

protected:
    Address(Type type) : _type(type) {}

private:
    Type _type;
};

class InternalAddress : public Address {
public:
    InternalAddress(const string& portName) : Address(Internal), _portName(portName) {}
    ~InternalAddress() {}

    const string& portName() const { return _portName; }
private:
    string _portName;
};

class ExternalAddress : public Address {
public:
    ExternalAddress(const string& handlerName) : Address(External), _handlerName(handlerName) {}
    ~ExternalAddress() {}

    const string& handlerName() const { return string("handler"); }
private:
    string _handlerName;
};

I want to eliminate dynamic memory allocation so I created a factory function that returns one of these by value, depending on a parameter:

static Address addressForType(Address::Type type) {
    if (Address::Internal == type)
        return InternalAddress();
    else
        return ExternalAddress();
}

I then do something like this:

int main(int argc, char **argv) {
    Address address = addressForType(Address::External);
    cout << ((ExternalAddress&)address).handlerName() << endl;
}

Is there anything wrong with the what I'm doing here? Or, is this just a bad design?? The reason I ask is that I'm in the process of trying to modify a existing system so it uses less dynamic memory allocation. Changing the code to return by value, instead of a dynamically allocated object, causes a segfault when I try to access the string reference return by the "handlerName()" method.

If there's a better way to do this, please enlighten me! I'll add that, for brevity, I've left out copy constructor and assignment operators. Even with these present, I get the same result.

Thanks.

Your design is bad. Your code suffers from [object slicing](http://stackoverflow.com/questions/274626/what-is-object-slicing). — R Sahu, Jun 27 '16 at 16:22
Thanks! That's exactly what I was looking for. Redesign underway... — DaveR, Jun 27 '16 at 16:29
I think the main design problem is trying to avoid dynamic memory when using dynamic polymorphism: dynamic tools always require dynamic resources in some way. Either use static polymorphism, or give up on trying to avoid using dynamic resources — KABoissonneault, Jun 27 '16 at 16:36

score 7 · Answer 1 · edited May 23 '17 at 10:28

Is there anything wrong with the what I'm doing here?

Yes. What you are doing is called object slicing and will not give you the desired behavior. All you will have is a Address. It will no longer be a InternalAddress or ExternalAddress.

If there's a better way to do this, please enlighten me!

You are going to need some sort of dynamic memory allocation. To make you life easier and you code safer you should look into smart pointers which will manage the memory for you making it far less likely to have memory leaks.

score 3 · Answer 2 · answered Jun 27 '16 at 16:28

I want to eliminate dynamic memory allocation.....

Given that you're storing std::string objects in these classes, you're either not trying very hard, or misunderstanding what dynamic memory is in c++.

Anyways.

You appear to be operating under the assumption that C++ treats inheritance (and objects in general) as "Java, but with some value semantics", which is incorrect.

Code like the following does not return a ExternalAddress or InternalAddress polymorphically accessible, it returns a literal Address object, with little-to-no knowledge of what sub-class it might have tried to construct:

static Address addressForType(Address::Type type) {
    if (Address::Internal == type)
        return InternalAddress();
    else
        return ExternalAddress();
}

As a result, any type information and additional space required by those additional objects will be eliminated from the returned object, having been sliced up (as other comments/answers have alluded to).

If you're expecting proper polymorphism, you need to return objects by pointer (std::shared_ptr and std::unique_ptr are recommended for this purpose) or by reference.

//replace with std::shared_ptr and std::make_shared<> if you're expecting the pointer to be "owned" by multiple handles, like is common in Java.
static std::unique_ptr<Address> addressForType(Address::Type type) {
    if (Address::Internal == type)
        return std::make_unique<InternalAddress>();
    else
        return std::make_unique<ExternalAddress>();
}

@πάνταῥεῖ I'm not convinced. Logically, syntactically, yes: the `else` doesn't change the functionality of the program. It does make intent clearer though and it does make the program more readable. — Xirema, Jun 27 '16 at 16:30

score 1 · Answer 3 · edited May 23 '17 at 12:03

Your factory function should rather return a std::unique_ptr<Address>:

static std::unique_ptr<Address> addressForType(Address::Type type) {
    // ^^^^^^^^^^^^^^^^^^^^^^^^
    if (Address::Internal == type)
        return std::make_unique<InternalAddress>();

    return std::make_unique<ExternalAddress>();
}

As mentioned in the other answer you cannot avoid dynamic allocation anyways.

Also a factory should deliver ownership to the caller, hence std::unique_ptr is the correct choice for this case.

score 0 · Answer 4 · edited May 23 '17 at 12:10

Is there anything wrong with the what I'm doing here? Or, is this just a bad design?

Your function addressForType suffers from object slicing.

If there's a better way to do this

You need to return a pointer to an object allocated using heap memory and make sure to delete the returned object in the calling function.

static Address* addressForType(Address::Type type) {
    if (Address::Internal == type)
        return new InternalAddress();
    else
        return new ExternalAddress();
}

C++ Class Design and return by value

4 Answers4