Is "Value Validation in Getter/Setter" good style?

Question

my Getter/Setter methods check the value, before they set/return it. When the value is invalid, they throw an exception (BadArgumentException or IllegalStateException). This is needed since we initialize all members with invalid values - and so we avoid working with these invalid values ( == getting errors/segfaults/exception in other places).

The benefits are:

• When you receive a member-value from a model, you know that they are valid
• The validity checks are only performed in the model-object
• The value range is defined in the model-object

This seems to be very unusual, since most new team-member first complain about it - even if they agree with me after I explain it to them.

Question: Is this a good programming style? (Even if it wasts a bit of performance)

Example Code:

inline bool MyClass::HasGroupID const { return m_iGroupID != 0; }

int MyClass::GetGroupID() const
{
    if( !HasGroupID() )
        throw EPTIllegalStateException( "Cannot access uninitialized group ID!" );

    return m_iGroupID;

}   // END GetGroupID() const

void MyClass::SetGroupID( int iGroupID )
{
    // negative IDs are allowed!
    if( iGroupID != 0 )
        throw EPTBadArgumentException( "GroupID must not be zero!", iGroupID );

    m_iGroupID = iGroupID;

}   // END SetGroupID( int )

Usage:

// in serialization
if( myObject.HasGroupID() )
    rStream.writeAttribute( "groupid", GetGroupID() );

// in de-serialization
try {
    // required attribute - throw exception if value is invalid
    myObject.SetGroupID( rStream.GetIntegerAttribute( "groupid" );
} catch( EPTBadArgumentException& rException )
{
    throw EPTBadXmlAttribute( fileName, rException );
}

Answer 1

I think it's common style, and it's certainly one of the core motivations for getter/setters in the first idea. However, in your particular example, I don't think they make sense and I think that in general there are often better alternatives:

If calling GetGroupID raises an exception if HasGroupID yields false, then this is a programmer error - so you should probably use an assert. In fact, why is it possible to have an object without a group ID in the first place? This sounds like a slightly inelegant design.

Furthermore, if only non-zero IDs are valid, then you should try to enforce this restriction even earlier, using a dedicated type. That way, all the code dealing with group IDs can be sure that the IDs are valid (in fact, the compiler enforces this!) and you can't possibly forget to check in some place. Consider:

class GroupID {
public:
    explicit GroupID( int id ) : m_id( id ) {
        if ( m_id == 0 ) {
            throw EPTBadArgumentException( "GroupID must not be zero!", m_id );
        }
    }

    // You may want to allow implicit conversion or rather use an explicit getter
    operator int() const { return m_id; }
};

Using this, you can just write

void MyClass::SetGroupID( GroupID id )
{
    // No error checking needed; we *know* it's valid, because it's a GroupID!
    // We can also use a less verbose variable name without losing readability
    // because the type name reveals the semantics.
    m_iGroupID = id;

} // END SetGroupID( int )

In fact, since group IDs are now a dedicated type, you could rename your function to just Set and use overloads. So you have MyClass::Set(UserID) and MyClass::Set(GroupID) and so on.