Storing a pass-by-reference parameter as a pointer - Bad practice?

Question

I recently came across the following pattern in an API I've been forced to use:

class SomeObject
{
public:
    // Constructor.
    SomeObject(bool copy = false);

    // Set a value.
    void SetValue(const ComplexType &value);

private:
    bool         m_copy;
    ComplexType *m_pComplexType;
    ComplexType  m_complexType;
};

// ------------------------------------------------------------

SomeObject::SomeObject(bool copy) :
  m_copy(copy)
{
}

// ------------------------------------------------------------

void SomeObject::SetValue(const ComplexType &value)
{
    if (m_copy)
        m_complexType.assign(value);
    else
        m_pComplexType = const_cast<ComplexType *>(&value);
}

The background behind this pattern is that it is used to hold data prior to it being encoded and sent to a TCP socket. The copy weirdness is designed to make the class SomeObject efficient by only holding a pointer to the object until it needs to be encoded, but also provide the option to copy values if the lifetime of the SomeObject exceeds the lifetime of a ComplexType.

However, consider the following:

SomeObject SomeFunction()
{
    ComplexType complexTypeInstance(1);  // Create an instance of ComplexType.

    SomeObject encodeHelper;
    encodeHelper.SetValue(complexTypeInstance); // Okay.

    return encodeHelper;

    // Uh oh! complexTypeInstance has been destroyed, and
    // now encoding will venture into the realm of undefined
    // behaviour!
}

I tripped over this because I used the default constructor, and this resulted in messages being encoded as blank (through a fluke of undefined behaviour). It took an absolute age to pinpoint the cause!

Anyway, is this a standard pattern for something like this? Are there any advantages to doing it this way vs overloading the SetValue method to accept a pointer that I'm missing?

Thanks!

Answer 1

I'd say yes it is standard to have a reference that is taken as a pointer (as in I have seen it in multiple projects) and yes it is bad as it does hide the problem of object lifetime.

And the const cast is unnecessary and completely horrible. At the very least it should be a ref that is passed in, not a const ref.

As you suggest one way of improving it would be better to have 2 different functions one that takes a const ref and copies (setting the copy flag at the same time), and the other that takes a pointer, so the user is forced to explicitly call the non copy behaviour.

Better would be to have 2 different methods that have different names (maybe CopyValue and ReferenceValue), and better still would be to have a base class with 2 different sub classes classes that do the 2 different behaviours.

Answer 2

Another solution would be to replace the method that takes a reference with one that takes a pointer. The caller would then create the object using the new operator and store it in a unique_ptr (I would say auto_ptr, but the very reason it was deprecated is what we would end up using here). That way, you neither make copies, nor worry about destruction of the object.

A quick mash-up would be

class SomeObject
{
public:
    // Constructor.
    SomeObject();

    // Set a value.
    void SetValue(unique_ptr<ComplexType> pValue);

private:
    std::unique_ptr<ComplexType> m_pComplexType;
};

// ------------------------------------------------------------

void SomeObject::SetValue(std::unique_ptr<ComplexType> pValue)
{
    m_pComplexType = std::move(pValue);
}

.. which can be used as:

SomeObject SomeFunction()
{
    std::unique_ptr<ComplexType> pobjComplexTypeInstance= new ComplexType(1);  // Create an instance of ComplexType and store it in a unique_ptr

    SomeObject encodeHelper;
    encodeHelper.SetValue(pobjComplexTypeInstance); // Pass the unique_ptr, it will be copied inside the function

    return encodeHelper;
}