Beware of Incorrect Usage in Accessor Methods

When people have looked at my code, specifically my test code, one of the most common things they ask is why I test my getters and setters. They see this as a weird thing to do, but I tend to be a very paranoid defensive programmer, so I like to ensure that my getters and setters aren't actually modifying anything.

"That is paranoid, Scott" you proclaim, and try to enlighten me on all the code that doesn't modify accessor methods. But I've been burned by this assumption often, and a simple and stupid unit test ensures that the code is adhering to my assumptions. It's quick and painless.

Example 1: Ruby on Rails' has_secure_password utility uses an setter for password that modifies the object in an unintended manner.

It may be that the writer of this method did not intend #password to be used as a setter, but the name of the method makes me (the programmer) think it is a setter method.

The issue came about when I was making a Change Password form for my project, where the form takes input fields called old password, new password, and new password confirmation. I wanted to thin out my controller and instead put the ability to functionality to change a user's password in my model. The model ended up looking like this:

Since I wanted the form to report whether a validation error took place in any of the three input fields, I needed to first validate the new password (and its confirmation). Unfortunately, when you use the password setter method, the #authenticate method no longer works correctly because it uses the password_digest method in its comparison routine, and that variable is modified when you set the password accessor. That's a big no-no in my book.

Example 2: When I was working on the EA Forums website code, which is written in JForums, one thing I tripped on many time was the modification of values inside the getters and setters. See this code in User.java which modifies the registration date coming out of the object in a formatted string, but was set into the object as a Date. There is no way to get the raw, unformatted, registration date out of the object. This breeds distrust with the developer using the code because they cannot be sure which accessors are correctly written and which will bite them in the butt when using them in a view template.

These are just two examples but I've stumbled across many more in my career. My advice for when you see this is to start writing some simple unit tests to ensure that the accessor methods are storing and retrieving data as you expect, and then to attempt to rewrite (via pull requests or whatever) the accessor methods that violate the developer's trust into separate utility methods. In the above Java example, renaming the getter method to getFormattedRegistrationDate() and then creating a proper getter method that doesn't modify the variable would be sufficient to inform the developer that nothing weird is going on behind the scenes.