current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 10 down vote favorite
2

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<typename T> 
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.

share|improve this question
    
Use const T& and T&& for construct and assignment operators. Also use const T& where you don't modify the values. Use variadic templates with perfect forwarding for versatile in place constructors. – CodeAngry Nov 5 '14 at 14:55
up vote 5 down vote accepted

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 idium 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<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.

Also operator== should be const.

Cast and get methods should be returning a reference (big T objects will cost on a 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 need for serialization is already marked as mutable (ie 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.

share|improve this answer
    
Good comments, thank you. As for your general comment, I agree in general, however, this is something I need to fit into an ancient (pre-standard) codebase. Nothing has a serialization operator, they have external classes to serialize, with the pointless opaque getters and setters to get the state of the object. The idea is that these objects are serialized into ini files (yeah, I know, not my idea either) which are edited by humans later, and they want as few clutter as possible in it. I have some int fields which should work like this and I think this is the cleanest possible way to do this. – Tamás Szelei May 16 '13 at 12:59
2  
Actually, there's a point in using std::swap;--in fact it's a well-known idiom used to enable ADL: stackoverflow.com/questions/3279543/… ; See also: en.wikipedia.org/wiki/Argument-dependent_name_lookup and Section 3.II. in boostpro.com/writing/n1691.html – Matt May 16 '13 at 18:04
    
@Matt: yes. I keep forgetting that. – Loki Astari May 17 '13 at 7:31

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.