views:

255

answers:

9

I have the following class

class CItem
{
 public:
  CItem(CRegistry &Registry) _Registry(Registry) {Registry.Register();}
  ~CItem() {_Registry.Unregister()};


 private:
  CRegistry &_Registry;
}

After a while it turns out that not all CItem objects need to be registered so I need a version of CItem which does not requires Registry in constructor (and of course of the registration code). How can I implement this? The only solution I can see here is to get and keep Registry as a pointer. Is there more elegant solution, like using templates, etc (I don't like switching from reference to pointer)?

+5  A: 

The only other alternative would be to create a CItem base class and then derive ItemWithRef (which has a reference) and ItemWithoutRef from it. But ir's much easier and clearer to use a pointer.

From the number of questions regarding references as members, it seems that somewhere, someone is spreading the view that references are good and pointers are bad. This is simply not the case, particularly when it comes to data members.

And just to clarify the underscore thing:

  • names that begin with an underscore and a lowervcase letter are reserved for the C++ compile/library writers when they occur at namespace (i.e. outside of classes) scope

  • names thatb begin with an underscore and an uppercase letter, or contain two consecutive underscores are unconditionally reserved for the compiler/library writers - you must not use them in your own code

anon
+1  A: 

Could you create another constructor that doesn't take a CRegistry as a parameter:

CItem();

that initializes _Registry to a static "zombie" value? Probably less elegant than using a pointer or subclassing?

Bill Carey
To do that you have to be able to create a "null" Registry object, which will probably entail changes to the Registry class. This is s very bad solution, IMHO.
anon
True. Not one that I personally would implement. I'd go with a private pointer.
Bill Carey
+1  A: 
  1. Use inheritance: Make a base class CItem and derive CRegisteredItem from this.
  2. Use pointers (and an overloaded constructor)
Dario
+1  A: 

One obvious alternative to a pointer is a private: static CRegistry selfRegistered;. Youn can then write CItem::CItem() : Registry(selfRegistered) { }

MSalters
A: 

You could make CRegistry a singleton (or simply just a standalone class) and decide in the CItem constructor whether you want to register that particular instance. This decouples the items from the registry and IMHO makes it easier to change things in the future.

Kristo
A: 

You can probably use template specialization like this:

class CRegistry
{
public:
    void Register(){}
    void Unregister(){}
};

template <class RegistryType>
class CItem
{
        public:
                CItem(){}
                ~CItem() {}


};


template<>
class CItem<CRegistry>
{
    public:
     CItem(CRegistry &Registry_in):  Registry(Registry_in) {Registry.Register();}
    ~CItem() {Registry.Unregister();}


        private:
                CRegistry& Registry;
};

int main()
{

    CRegistry c1;
    CItem<CRegistry> it(c1);
    CItem<int> it2;
        return 0;
 }
Naveen
+4  A: 

If you want to keep a single class, just change the attribute into a raw pointer and allow it to be null. As Neil points out, there is a widespread unjustified jihad against raw pointers that is not fully justified. Use a raw pointer and clearly document (comment) that the object holds no ownership of the pointed memory so that no one feels like adding a delete in your destructor at a later time.

All other solutions will be worse than using a pointer internally. It is an implementation detail. Also consider whether it makes sense. Your code will no longer be able to assume that the pointer is valid and it will complicate the logic inside your class.

class CItem
{
public:
   CItem(CRegistry &Registry) : _Registry(&Registry) {Registry->Register();}
   CItem() : _Registry(0) {}
   ~CItem() { if ( _Registry ) _Registry->Unregister(); }

private:
   CRegistry *_Registry; // Pointer is not owned. Do not delete!
};

As a last note: do not prefix attributes with a single underscore as they are reserved by the standard for C++ implementations (compiler and standard libraries)

David Rodríguez - dribeas
"do not prefix attributes with a single underscore" and in case it isn't obvious, don't use multiple underscores either. Names containing a double underscore anywhere in them are reserved for all uses.
Steve Jessop
To be more explicit about the optional semantics, you could use boost::optional rather than a raw pointer. But otherwise I agree, for "built-in" optional support, pointers do the trick nicely.
jalf
+1  A: 

By having a reference member, you are clearly saying that a registry is needed for every CItem (because a reference must be bound to a valid object, and every CItem has a reference member). The direct way to implement an optional CRegistry is to use boost::optional (this is safer and clearer than the NULL pointer idiom). Alternatively, the Null Object Pattern lets you have a CNullRegistry class which implements registration, deregistration, and other functions, as no-ops. Then make the constructor parameter default to a CNullRegistry object.

However, you might like to consider a higher level approach which clearly delineates registered CItems from unregistered ones. As other answers suggested, inheritance and template specialization both provide mechanisms for that. The advantage is that your design can now depend on the "I'm registered" invariant.

A: 
Tobias