I maintain a small application for my bowling league. Its main features are entering scores and printing reports such as team standings and player averages. It’s nothing complex. I'm currently making enhancements to it. As I was implementing a feature, I wrote the following code
if (SelectedBowler.Id == 0) to see if the bowler was new.
I sat back and looked at what I wrote. I've always heard that getters and setters are a bad idea, but I never understood why. If you have ever been to a Code Retreat then you learned about the four simple rules of design. Using these rules, I'll explain why you should avoid using getters and setters. The four simple rules of design are:
- All unit tests pass
- Say everything once and only once (Don't Repeat Yourself)
- Express every idea you need to express
- Remove all superfluous parts (YAGNI)
Before We Begin
I think that getters and setters are evil because developers create getters and setters without thinking about the behavior of the data. Getters retrieve private data for business logic that ends up being duplicated, while setters set the value of the object's private data instead of setting it through some behavior. Over time, the use of getters and setters obscure the code making future maintenance difficult.
I understand in the .NET world that there is a lot of use of getters and setters. Sometimes getters (maybe setters) are needed for objects such as data transfer objects. However, I encourage you to minimize your use of them. When you do your application will become easier to maintain.
Getters
The first rule states that all unit tests should pass. That was easy because I test for the ID being zero. However, this isn’t good because whenever the code changes, my tests change. This leads into the second rule, "say everything once" or more commonly known as, "don't repeat yourself". The above code is used to determine if the bowler is a new bowler in the league. Anytime I need to know if the bowler is new, then I need to write the above code, thereby repeating code. The third rule is a very important rule; express every idea you need to express. Do you know what I meant by the above code? You might, if you have used a database. Maybe you have written something similar in your time. However, what if a new person joined the team? Would they know what you meant? If what you write now is not clearly expressed, then the future of the code will cause confusion to the future developers. Lastly, my code does have superfluous parts; the ID. It’s not necessary to expose that value to the consumer of the class. When it comes to this rule we aim to reduce unneeded code. The class needs to know about the ID, but no one else does.
A clearer way to write the above code is
if (viewModel.IsNewBowler).
Inside IsNewBolwer you can have the logic to determine if the bowler is a new bowler,
public bool IsNewBowler { get { return SelectedBowler.Id == 0; } }.
If the logic to determine if the bowler is new changes, then I have to update only one place in the code. My tests will be useful because they will be testing the behavior and not the value. I no longer have to keep writing if ID is equal to zero. It’s conveniently wrapped within a property. In addition, it states clearly the intent of the property. This will make future enhancements easier because the developer will understand the intent of the code.
Setters
Setters are similar to getters in the sense that they violate the same rules. Have you ever seen code similar to the following?
object.property = someValue or object.property.property = someValue
Sticking with the four simple rules of design, you'll see how using setters makes the application brittle. Making all unit tests pass might not be violated because the developer can make unit tests pass. However, the unit tests will always change when the internal structure of the class changes. Setters violate the "don't repeat yourself" rule because the developer will always need to set the properties when a behavior is applied. By setting properties, the intent of the code is not clearly stated. You know what the code means now, but you might not remember a year or two down the road. Other developers might not know what you mean either.
In my bowling league there is only one way for a bowler to get on a team; when they are drafted. But for the sake of this post, let's assume that bowlers can be traded to other teams throughout the season. In order for a bowler to change teams, the code is rather easy bowler.Team = newTeam
But this code makes my application more brittle. Do my unit tests pass? They do, but as I've stated above, if the internal workings of the class change then my unit tests will have to change with them. I have to repeat the code in order for the bowler to be placed on a team; when the bowler is drafted or when the bowler is traded. You might argue that this is ok, since its one value, but what if you were setting multiple values? By setting the Team property, my intent is not clearly stated. If I set the Team property, is the bowler being drafted or traded? This becomes superfluous because I have to make the same calls any time the bowler is put on a new team. A more descriptive way to express the intent is to write bowler.DraftedBy(team) and bowler.TradedTo(team)
What Else
Do you remember data encapsulation? It's one of the principles of object oriented design. Data encapsulation hides the private data of a class. In the first pass of the code, the private data isn't encapsulated. It is freely available for anyone to use. Given that it's wrapped in a property doesn't mean that the data is encapsulated. The private data is still visible. Exposing private data will make future maintenance of the application more difficult. I read this post about getters and setters to further understand the problem. In this article, the author states that using getters creates an unnecessary coupling. That's because you are dependent on the internals of the class. When those internals change, then your code changes with it.
