Is it good practice to use pointers as class members?

Question

I'm fairly new to C++, and I'm trying to understand the good practices for building classes.

Let's say I have a class Foo:

class Foo {
  public:
    double foo;
    Foo(double foo);
    Foo add(Foo f);
}

I want to make a class Bar, that is made of two Foo objects, and that creates a third one at construction.

1st option: Objects as class members

class Bar {
  public:
    Foo foo1;
    Foo foo2;
    Foo foo3;
    Bar(const Foo &f1, const Foo &f2);  
}

Bar::Bar(const Foo &f1, const Foo &f2):
{
  foo1 = f1;
  foo2 = f2;
  foo3 = f1.add(f2);
}

As is, it does not work as I have not defined a default constructor for Foo.

2nd option: Pointers as class members

class Bar {
  public:
    const Foo* foo1;
    const Foo* foo2;
    const Foo* foo3;
    Bar(const Foo &f1, const Foo &f2);  
}

Bar::Bar(const Foo &f1, const Foo &f2):
{
  foo1 = &f1;
  foo2 = &f2;
  foo3 = &(f1.add(f2));
}

Note: I have to declare foo1 and foo2 as const for the constructor to work. It still fails though because for foo3 I am taking the address of a temporary result, which is illegal.

---

Which option is more natural (and how can I fix the errors)? I feel the first option is probably better, but then my Foo objects have to be created twice in memory, don't they? (once to call the constructor, and a second time by the constructor itself)

Any help appreciated.

Answer 1

It's fine to use pointers as members, but in your case you are simply working around a minor hiccup that really doesn't warrant the use of pointers, and using pointers can be dangerous as evidenced by an issue I'll point out shortly.

As is, it does not work as I have not defined a default constructor for Foo.

This is easily resolved by using initializers for Bar:

Bar(const Foo &f1, const Foo &f2) : foo1(f1), foo2(f2), foo3(f1.add(f2)) {}

as demonstrated here:

#include <iostream>

class Foo {
  public:
    double m_foo;
    Foo(double foo) : m_foo(foo) {}
    Foo add(Foo f) { f.m_foo += m_foo; return f; } // returns temporary!
};

class Bar {
  public:
    Foo m_foo1;
    Foo m_foo2;
    Foo m_foo3;
    Bar(const Foo &foo1, const Foo &foo2);  
};

Bar::Bar(const Foo &foo1, const Foo &foo2)
    : m_foo1(foo1)
    , m_foo2(foo2)
    , m_foo3(m_foo1.add(m_foo2))
{
}

int main() {
    Foo foo1(20.0);
    Foo foo2(22.0);
    Bar bar(foo1, foo2);

    std::cout << bar.m_foo3.m_foo << "\n";

    return 0;
}

Live demo: http://ideone.com/iaNzJv

In your pointer solution you introduce a glaring pointer problem: a pointer to a temporary.

foo3 = &(f1.add(f2));

f1.add returns a temporary Foo, which you take the address of, and then it goes away. This is a dangling pointer.

Your pointer implementation also doesn't explicitly take pointers as its inputs so f1 and f2 could run into the same problem:

Bar(Foo(20), Foo(22));  // two temporary Foos passed by reference
                        // but having their addresses taken. ouch.

If you're taking pointers, it's best to do that at the api to your class; you're going to have to care about the lifetime of the things pointed to, and try to make it easier for a caller to tell that you are doing so.

Bar(Foo* f1, Foo* f2);

But now if you're going to have F3 you're going to be responsible for managing it's memory:

Bar(Foo* f1, Foo* f2)
    : foo1(f1), foo2(f3), foo3(new Foo(*f1.add(*f2)))
{}

~Bar()
{
    delete f3;
}

So in your example case, using members is probably drastically better.

Save the use of pointers for large objects that you definitely don't want to copy, and where you can't use a move operation.

--- EDIT ---

The problem of conveying ownership of pointers has been largely solved in modern C++ (C++11 and higher), through "smart pointers", in particular std::unique_ptr and std::shared_ptr.

It is generally considered Best Practice to use these instead of raw pointers, although it requires learning some newer C++ concepts.

#include <memory>

struct Foo {};
class Bar {
public:
    std::unique_ptr<Foo> m_f1; // we will own this
    std::unique_ptr<Foo> m_f2; // and this

    Bar(std::unique_ptr<Foo> f1) // caller must pass ownership
        : m_f1(std::move(f1))    // assume ownership
        , m_f2(std::make_unique<Foo>()) // create a new object
    {}

    ~Bar()
    {
        // nothing to do here
    }
};

int main() {
    auto f = std::make_unique<Foo>();
    Bar(std::move(f)); // the 'move' emphasizes that
                       // we're giving you ownership
    // 'f' is now invalid.

    return 0;
}

Live demo: http://ideone.com/9BtGkn

The elegance of this is that when Bar goes out of scope, the unique_ptrs will ensure that the objects they own are destroyed for us -- we don't have to remember to delete them.

In the above example, it would probably have been much better to make m_f2 a member rather than a pointer.

Answer 2

If the objects are not too expensive to pass around, I suggest using objects as members.

If you need to use pointers for some reason, you need to have ownership policies in place. Does Bar object own the objects? Does Bar just holds the pointers to the objects but is not responsible for releasing resources used by them?

If Bar owns the Foo objects, prefer to use one of the smart pointers. You'll need to make copies of those objects by using new and hold on to those pointers.

Here's how I see it:

class Bar {
  public:
    std::unique_ptr<Foo> foo1;
    std::unique_ptr<Foo> foo2;
    std::unique_ptr<Foo> foo3;
    Bar(const Foo &f1, const Foo &f2) : foo1(new Foo(f1)), ... {}
};

std::unique_ptr does not have a copy constructor. Hence, you must provide a copy constructor for Bar and initialize its members from the copy appropriately.

If Bar does not own the Foo objects, you might be able to get by using references as member data.

class Bar {
  public:
    Foo const& foo1;
    Foo const& foo2;
    Foo const& foo3;
    Bar(const Foo &f1, const Foo &f2) : foo1(f1), ... {}
};