HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simple "nullable" template class. Are there weaknesses in the implementation?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simpletemplateweaknessesthearenullableimplementationthereclass

Problem

The goal is to have fields in a class which are optional and can be determined if they are set in order to be able to leave them out of serialization.

template 
class Nullable
{
public:
    Nullable() : m_value(), m_isSet(false) {}
    Nullable(T value) : m_value(value), m_isSet(true) {} 
    Nullable(const Nullable& other) : m_value(other.m_value), m_isSet(other.m_isSet) {}

    friend void swap(Nullable& a, Nullable& b)
    {
        using std::swap;
        swap(a.m_isSet, b.m_isSet);
        swap(a.m_value, b.m_value);
    }

    Nullable& operator=(Nullable other) 
    {
        swap(*this, other);
        return *this;
    }

    T operator=(T value) { set(value); return m_value; }
    bool operator==(T value) { return m_isSet && value == m_value; }
    operator T() const { return get(); }

    T get() const 
    {
        if (m_isSet) return m_value;
        else throw std::logic_error("Value of Nullable is not set.");
    }

    bool is_set() const { return m_isSet; }

    void reset() { m_isSet = false; m_value = T(); }

private:
    void set(T value) { m_value = value; m_isSet = true; }

private:
    T m_value;
    bool m_isSet;
};


Any comments are welcome.

Solution

Don't see the point in:

using std::swap;


It adds a line. It is also longer to type than just prefix each swap with std::

As pointed out by @Matt below. There is a good reason to do this.

Read his links below for a more detailed description.

No point in doing copy-swap idiom for a class that is not managing resources.

So really there is no need for the copy constructor or the assignment operator: the compiler-generated versions will work perfectly well.

If you can compare against a value:

bool operator==(T value) { return m_isSet && value == m_value; }


Why can't you compare against Nullable? The cast operator does not help you here - operator T() const { return get(); } - as it potentially throws. Yet it is still logical to be able to compare or it should throw if either side has m_isSet as false.

Nullable   x(5);
Nullable   y;

if (y == x){} // works as expected (always false as y is not set).
if (x == y){} // throws an exception.


Also operator== should be const.

Cast and get methods should be returning a reference (big T objects are expensive to copy). You will also want const versions of these functions.

On a reset:

void reset() { m_isSet = false; m_value = T(); }


Why reset the m_value when it can not be accessed anyway when m_isSet is false. Also the cost of setting T here may be expensive for some classes.

As a general comment.

I don't see the point. Each class should have its own serialization operators (>> and <<) and you will need to serialize the whole state for this to work symmetrically. Anything that is not needed for serialization is already marked as mutable (i.e. it is not part of the logical state of the object). Everything else is part of the logical state of the object. Anything that is optional has its own serialize operator and understands when not to serialize its own state.

Code Snippets

using std::swap;
bool operator==(T value) { return m_isSet && value == m_value; }
Nullable<int>   x(5);
Nullable<int>   y;

if (y == x){} // works as expected (always false as y is not set).
if (x == y){} // throws an exception.
void reset() { m_isSet = false; m_value = T(); }

Context

StackExchange Code Review Q#26224, answer score: 6

Revisions (0)

No revisions yet.