tags:

views:

174

answers:

6

Say I have an object of type A having Initialize() method. The method receives object B, which are kept as the object data member. B object is shared between several objects, thus A should contain the originally received B object and not its copy.

   class A 
   {
       public:
          bool Initialize(B?? b);
       private:
          B?? m_B;    
    }

Object B must present. Thus I think to pass it by reference, instead of passing by pointer and failing Initialize() in case B is NULL.

The only alrernative for m_B is to be of type of pointer B (it cann't be reference of B, since the intializating of B is not done in A c-tor). Thus Initialize should look like:

bool A::Initialize(B& b) 
{
    m_B = &b;
    ....
}

Is this aproach is ok?

UPD:

  1. The code is not new and I'm just trying to "fix" some problems. Actually I'm not talking about some concrete A and b classes, rather about a way the problem is approached in my code base. The code widely passes pointer to B and verifying it in Initialize() if it's NULL.

  2. Passing B to A's c-tor is not always a good option too. There're also other parametrs passed to A, which are not exists at A creation time. Therefore I woudln't prefer to pass part of parameters to A c-tor and the rest to A::Initialize().

  3. shared_ptr can be "NULL" too, so passing it to A::Initialize() not different from passing just pointer to B, in that aspect that signature of Initialize() dosn't declare if B is mandatory or not. In my case it is and I want to express it by passing reference to B.

  4. Our code currently is not using boost at all. So, although shared_ptr better solution than just passing raw pointer, can solution proposed by me be considered as bad, but still solution.

A: 

I'd go with a boost::shared_ptr. If you use references you might have to worry about the scope of the referent, if you use a plain pointer you get into trouble with memory managment. Who is going to delete B?

Maybe just reflect on your scheme again: Do you really need to share the object or is a copy ctor for type B enough? If B is going to be changed by other classes and those changes need to be known by other classes, you need a shared_ptr.

pmr
How do you know it is dynamically allocated?
anon
+1  A: 

If B is optional then it can be represented as a pointer. If B is required then it should be represented as a reference.

If possible try and avoid "two stage construction" with the initialise method. If that can't be done then internally to A you need to treat B as optional and so store it as a pointer and test wherever you might want to use it.

If your initialise method (or, ideally, constructor) requires a B then you should pass it in as a reference.

This all assumes that you know who actually owns B; perhaps B owns the instances of A and initialises them with references to itself or perhaps B is owned by something that also owns all of the instances of A that refer to this instance of B.

If the objects of A own B jointly then you should use something like a boost::shared_ptr to make the shared ownership explicit; assuming that B is dynamically allocated with new.

Len Holgate
I disagree. You're confusing how you wire up the objects with their lifetime. Both should be explicit and both need not be related. Saying that it should be a pointer because it's a pointer somewhere and it may become invalid is spurious; everything can be represented by an address (pointer) and everything can become invalid if you don't manage the lifetime of it. In this situation the code is being given a reference to an object that must exist; so use a reference. Inside the code due to the two stage init the object is optional, so use a pointer that can either be a valid object or null.
Len Holgate
Hmm, and now that the previous comment has been deleted it seems I'm talking to myself...
Len Holgate
Sorry about that. I simply felt that the tone of my comments become somewhat rude and removed them. Maybe too quickly.
Tomek Szpakowicz
No worries. I often find I'm talking to myself ;)
Len Holgate
+1  A: 

I'd stay with pointer. Reference here just sends wrong message.

You don't use references to object in situations when you plan to take pointer to object and keep or share it, etc. Main reason for references in C++ is allowing things like operator overloading and copy constructors to work for user defined types. Without them it would be difficult to provide this functionality with syntax that doesn't differ from built in types.

But in this situation you're not trying to mimic built in type. You are operating on object which is normally used through pointer and even shared through several different pointers. So be explicit about that.

As for b being NULL, by all means use assert(b) (or similar construct) to enforce contract and stop invalid program. (I wouldn't throw exception though. Even if you forget about problems with exceptions in C++, are you planning to ever catch and handle such exception in your code?) I would also add such assertions to all code that uses m_B in case someone forgot to call A::Initialize().

Using reference to ensure pointer is not null, might misfire. In most implementation you can make reference from NULL or dangling pointer without raising any error. Your application will fail only if you try to use this reference. So if someone accidentally passes you B *pb which equals NULL, you could call pa->Initialize(*pb) and nothing happens. Except that pa->m_B is now NULL.

Whether to use something like boost::shared_ptr is up to you and your strategy of memory management. It's a completely unrelated issue.

Tomek Szpakowicz
Sorry, I completely disagree. Passing a pointer says "this can be optional". Suggesting that you assert for null just means that you need to document the fact that the function, though taking a pointer, must take a pointer that cannot be null. Accepting a reference says "this is not optional"; your argument for not using a reference because it might be null anyway is, IMHO, most likely to have been a problem for you simply because your designs are broken because you take a pointer where you should take a reference and you aren't enforcing the optional or required nature of your parameters. ;)
Len Holgate
Relax, sometimes people disagree with you.
Len Holgate
You're right.I don't really know why I even argue about it.I stated my opinion, provided some arguments and it should be it.I'm sorry for my later comments being somewhat rude and pointless. I should better remove them.
Tomek Szpakowicz
tomekszpakowicz; s'ok, 'designs are broken' is probably a bit harsh ;)
Len Holgate
A: 

I believe you should use a constructor instead of using a Initialize method.

Passing B to A's c-tor is not always a good option too. There're also other parametrs passed to A, which are not exists at A creation time. Therefore I woudln't prefer to pass part of parameters to A c-tor and the rest to A::Initialize().

Have you heard about preconstruction?

Here is an example of what you could of done instead:

class IsEmpty{};
class A
{
    B *b_;
    int *c_;
    char *d_;

    void Initialize(B *b)
    {
     b_ = b;
     c_ = b_->Getc();
     d_ = b_->Getd();
    }

public:
    A(B *b)
    : b_(0), c_(0), d_(0)
    {
     if(b == 0) throw IsEmpty();   
     Initialize(b);
    }
};
Partial
A: 

Passing B as reference says the the lifetime of B is longer than the lifetime (or time to deinitialization) of A. If that is the case, you should pass by rerefence. Internally you can also store the reference in a Boost reference wrapper (but this is Boost, so maybe not an option).

If you pass pointers, and you are sure, that they should never be NULL if your program is correct, then use assertions rather that an if-clause to check for that.

I too sometimes have the szenario you desrcibe and usually use your proposed solution. It's the simplest one.

Depending on the complexity and design of your classes, you can also use a variation of the state pattern, where the state-object describing the "initialized"-state is constructed in the initialize-method. In that case, you could pass the reference through to the constructor of the state-object. This probably overkill (state pattern in C++ has considerable boiler plate, so make sure it's worth it) and requires a lot of refactoring, but maybe it helps you in some way.

Space_C0wb0y
A: 

I'd definitely avoid using a reference as a member of a class if at all possible. There's a million ways this can screw you up. If you do so you should use initializer list, but that is another feature you should avoid like the plague. It works fine for simple cases but it can cause serious problems.

Charles Eli Cheese