patterncppMinor
Simple "nullable" template class. Are there weaknesses in the implementation?
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.
Any comments are welcome.
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:
It adds a line. It is also longer to type than just prefix each swap with
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:
Why can't you compare against
Also
Cast and get methods should be returning a reference (big
On a reset:
Why reset the
As a general comment.
I don't see the point. Each class should have its own serialization operators (
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.