Is Getter / Setter Validating Values ​​a Good Style?

Question

my getter / setter methods check the value before setting / returning it. When the value is invalid, they throw an exception (BadArgumentException or IllegalStateException). This is necessary because we initialize all members with invalid values ​​- and therefore we avoid working with these invalid values ​​(== getting errors / segfaults / exception in other places).

Benefits:

• When you get a member value from a model, you know that they are valid.
• Validation checks are performed only on the model object.
• The range of values ​​is determined in the model object.

This seems very unusual, as most of the new team members first complain about it, even if they agree with me after I explain to them.

Question: Is this a good programming style? (Even if it is slightly different from performance)

Code example :

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 )

Using

// 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 this is a common style, and this is definitely one of the main motives for getter / seters in the first idea. However, in your specific example, I don’t think they make sense, and I think that in general there are often better alternatives:

GetGroupID , HasGroupID false, , , , assert. , ? .

, , , . , , , , ( , !), - . :

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; }
};

,

void MyClass::SetGroupID( GroupID id )
{
    // No error checking needed; we *know* it valid, because it 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 )

, , Set . , MyClass::Set(UserID) MyClass::Set(GroupID) ..

Answer 2

, getter/setter. - , -. : - , , , ..

, , , , . / , , .

Answer 3

. , , .
, .
, .
, #, ++ - : -)

class MyClass{

    private int groupId; 

    public MyClass(int groupId) {
        if(groupId ==0){
            throw new ArgumentOutOfRangeException("groupId",groupId,"Group Id must be >0");
        }
        this.groupId = groupId;
    }

    public int GroupId{ 
        get{ 
             return groupId;
        }
    }
}

, MyClass , . , MyClass .
, MyClassChangeable

public class Programm {
    static void main(string[] args) {
        IMyClass myClassChangable = new MyClassChangable(-123);
        try {
            MyClass correctMyClass = new MyClass(myClassChangable);
            useThisClass(correctMyClass);

        } catch (Exception ex) {
            //Display to user or something
            Debug.WriteLine(ex);
        }
    }

    static void useThisClass(MyClass myClass) {
        //this is alway correct!
        int groupId = myClass.GroupId;

    }
}

interface IMyClass {
    int GroupId { get; }
}

class MyClass : IMyClass {
    private int groupId;

    public MyClass(int groupId) {
        if (groupId == 0) {
            throw new ArgumentOutOfRangeException("groupId", groupId, "Group Id must be >0");
        }
        this.groupId = groupId;
    }

    public MyClass(IMyClass instance)
        : this(instance.GroupId) {
    }

    #region Implementation of IMyClass

    public int GroupId {
        get { return groupId; }
    }

    #endregion
}


public class MyClassChangable : IMyClass {
    private int groupId = 0;

    public MyClassChangable() {

    }

    public MyClassChangable(int groupId) {
        this.groupId = groupId;
    }

    #region Implementation of IMyClass

    public int GroupId {
        get { return groupId; }
        set { groupId = value; }
    }

    #endregion
}

, , "" ( ). "BusinessCore" MyClass, . OutOfBounds!
, , , , - , !
!

Answer 4

: Setter , (, ). Getters ( - ), Debug , .

, , . Invariant (, GroupId), GroupId ... ?? .

Getter, : (, Boost.Optional). , , ... .