views:

258

answers:

4

C++ newbie question. Please, verify I'm doing it right.

I have a global application class spawning it's little kids and I need to give the kids access to some of the application facilities. So I decided to pass them to children by reference.

I tested the idea as show below. It seems to work fine. I just wanted to make sure I'm not doing something dangerous. Might be there any pitfalls I overlooked?

Dad creates children and gives them his car keys:

#include <iostream>
using namespace std;

class CCarKeys
{
    public:
        CCarKeys(const string& Name) : _Name(Name) {}
        string _Name;
};

class CChild
{
    public:
        CChild(CCarKeys& CarKeys) : _Name("Child"), _CarKeys(CarKeys) {}
        string _Name;
        CCarKeys& _CarKeys;
        void TestHasKeys() {cout << "I got " << _CarKeys._Name << endl;}
};

class CDad
{
    public:
        CDad() : _Name("Dad"), _HondaCarKeys("Honda keys"), _ChevyCarKeys("Chevy keys") {}
        string _Name;
        CCarKeys _HondaCarKeys;
        CCarKeys _ChevyCarKeys;
        CChild *_Boy;
        CChild *_Girl;
        void MakeBoy() {_Boy= new CChild(_HondaCarKeys);}
        void MakeGirl() {_Girl= new CChild(_ChevyCarKeys);}
};

int main ()
{
    CDad Dad;

    Dad.MakeBoy();
    Dad.MakeGirl();
    Dad._Boy->TestHasKeys();
    Dad._Girl->TestHasKeys();
}
A: 

Looks good to me (if keys is all they need). They might need some other services from Dad which are requested later - like:

Wallet += MyDad.GasMoney(REQUEST_MAX_AND_PROMISE_TO_BE_HOME_BY_10PM) ;

But they don't have a reference to Dad, so they won't be able to do that. So I would have the CChild constructor take a this reference, too.

class ICashProvider {
public:
  virtual money Request(IPerson,CashRequestFlags) ;
};

class IChaffeur {
public:
  virtual void Drive(IPerson[]) ;
};

etc.

And then CChild constructor would need to take ICashProvider and IChaffeur, as would CWife and CGirlfriend (and CBoyfriend, perhaps). At this point, I think you might realize that this level of granularity is pointless in the face of Dad's responsibilities and you just give everyone this and have Dad authenticate requests by forcing callers to send their own this on some methods, so you don't have Dad performing incest or changing the CWife's diaper.

Cade Roux
If a child has _this_ pointer it could use dad's facilities which are supposed to be used only by mammy. It sounds scary to me. Isn't it better to only pass what's needed to a child?
Jack
Unfortunately, there is only one set of public services provided by a class, in order to have some of Dad's services consumable by different classes, I think you would need to break up Dad into different services and expose them as interfaces and pass just the interface to the kid, wife, etc.
Cade Roux
There are even movies about that - dads splitting their interface and passing different versions (eg. with overriden method getFamilyStatus()) to different objects. :-) I believe this is normally called the Proxy pattern
jpalecek
A: 

Passing by reference is quite the same that passing by pointer except for semantics and the fact that you can do nothing with the pointer itself if passing by reference.

Your code is OK.

Quassnoi
(const foo } and foo(F*a){ USE(a); }.
Mr.Ree
A: 

In your particular case, car keys aren't likely to be granted permanently, but rather requested as needed and granted on per-request basis. So it's more

class Dad
{
   /** may return NULL if no keys granted */
   CarKeys *requestKeys(CChild forChild);
}

In more genral case of main app class and children, how about creating the data object shared by app and children in main(), and passing thereferences to everybody.

Arkadiy
A: 

It's possible and in your code it's not causing harm. But it's dangerous, because if you copy CDad, then the keys and pointers will be copied along. The objects that the pointers will point to, and the reference inside those objects, however, will remain the same. If the original CDad object then goes out of scope, the references in the objects referenced by the pointers are dangling, referencing no valid object anymore.

Maybe you can to reverse lifetimes: Create the keys on the heap, and the kids as normal members within the class. So if you copy dad, the kids are copied, but the keys are not. I think keys are immutable, so you can share the same key among multiple kids.

This brings to another point: If your keys are reasonable small (read: not huge) and immutable (so you don't have update anomalies if you change one key, but not the others), consider creating it not on the heap too - so they are automatically copied along too, and pass them to the kids when they need the key. You can make them normal pointers of the kids, but i think that's ugly, because the kid does not contain a key - but uses it. So a pointer/reference or a function parameter fits well, but not a "real" data member.

If you are going with the shared key and the keys-on-heap, you should use smart pointers - because you have to keep track of all kids and dads. If the last kid/dad goes out of scope, you have to delete the key again. You use boost::shared_ptr for that:

class CCarKeys
{
    public:
        CCarKeys(const string& Name) : _Name(Name) {}
        string _Name;
};

class CChild
{
    public:
        CChild(boost::shared_ptr<CCarKeys> const& CarKeys) 
            : _Name("Child"), _CarKeys(CarKeys) {}
        string _Name;
        boost::shared_ptr<CCarKeys> _CarKeys;
        void TestHasKeys() {cout << "I got " << _CarKeys._Name << endl;}
};

class CDad
{
    public:
        // NOTE: null the kid pointers *if* you use raw pointers, so you can check whether
        // a boy or girl is present. Without nulling them explicitly, they have 
        // indeterminate values. Beware. shared_ptr's however will automatically
        // initialized to default "null" values
        CDad() : 
            _Name("Dad"), 
            _HondaCarKeys(new CCarKeys("Honda keys")), 
            _ChevyCarKeys(new CCarKeys("Chevy keys")) {}
        string _Name;

        boost::shared_ptr<CCarKeys> _HondaCarKeys;
        boost::shared_ptr<CCarKeys> _ChevyCarKeys;

        // also use shared_ptr for the kids. try to avoid raw pointers. 
        boost::shared_ptr<CChild> _Boy;
        boost::shared_otr<CChild> _Girl;

        void MakeBoy() {_Boy.reset(new CChild(_HondaCarKeys));}
        void MakeGirl() {_Girl.reset(new CChild(_ChevyCarKeys));}
};

// main can be used unchanged

Of course, you can avoid all this complexity by just making the CDad class non-copyable. Then you can use your original solution, just with making the kids use a shared_ptr and make the kids non-copyable too. Ideally, one should use a non-shared pointer, such as auto_ptr, but that auto_ptr has some pitfalls too, which shared_ptr all avoids:

class CCarKeys
{
    public:
        CCarKeys(const string& Name) : _Name(Name) {}
        string _Name;
};

class CChild
{
    public:
        CChild (CCarKeys& CarKeys) 
            : _Name("Child"), _CarKeys(CarKeys) {}
        string _Name;
        CCarKeys &_CarKeys;
        void TestHasKeys() {cout << "I got " << _CarKeys._Name << endl;}
    private:
        CChild(CChild const&); // non-copyable
        CChild & operator=(CChild const&); // non-assignable
};

class CDad
{
    public:
        CDad() : 
            _Name("Dad"), 
            _HondaCarKeys("Honda keys"), 
            _ChevyCarKeys("Chevy keys") {}
        string _Name;

        CCarKeys _HondaCarKeys;
        CCarKeys _ChevyCarKeys;

        // also use shared_ptr for the kids. try to avoid raw pointers. 
        boost::shared_ptr<CChild> _Boy;
        boost::shared_otr<CChild> _Girl;

        void MakeBoy() {_Boy.reset(new CChild(_HondaCarKeys));}
        void MakeGirl() {_Girl.reset(new CChild(_ChevyCarKeys));}
private:
    CDad(CDad const&); // non-copyable
    CDad & operator=(CDad const&); // non-assignable
};

If i had to implement such a class hierarchy, i would go with that solution, or just drop the keys as members, and pass/create them when needed to the children. Some other notes about your code:

  • Better drop the "_" from members or put them at the end or use some other notation. A name that begins with an underscore and is followed by an uppercase letter is reserved by C++ implementations (compiler, C++ std lib ...).
  • I personally find it confusing to have member names and variables start with an Uppercase letter. I've seen it only very rarely. But this isn't much to care about, it's just about personal style.
  • There is a famous rule (Zero-One-Infinity) that states when you got two things of something, you generally should be able to have arbitrary many things of that. So if you can have two children - why not have many of them? Two seems like an arbitrary choice. But it may have a good reason in your case - so ignore this when in your case it makes sense.
Johannes Schaub - litb
i'm in a hurry so i can't be bothered looking up what he talked about. can anyone enlight me? anyone doing so gets a cookie :)
Johannes Schaub - litb
Stay cool, guys. Thanks to everybody for the help.
Jack