The Evils of Getters and Setters

February 26th, 2013 Adamo Mosca, Consultant  (email the author)

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.

Be Sociable, Share!

Entry Filed under: Agile and Development

7 Comments Add your own

  • 1. belun  |  February 28th, 2013 at 10:27 am

    A nice and simple example of how to do it (how to move from getter/setter mentality to Encapsulation, thus proper OO code).

    You will find that, everything you did here, follows the principle “Tell, don’t ask”; therefor conforming to Information Hidding (the result of applying Encapsulation).

  • 2. Brian Gray  |  February 28th, 2013 at 8:14 pm

    It seems to me you are not so much railing against the methods or patterns themselves so much as mindlessly generating them. Sort of a “don’t just auto-generate your s/getters and let that be your model class design.”

    I tend to agree with you. I would rather expose an addFoo(Foo foo) method than a getFooList() method that returns the whole list. However there are plenty of cases where exposing getFirstName() on a Student object for example is completely reasonable. And I see no problem with that.

  • 3. Jim Halvorsen  |  February 28th, 2013 at 11:15 pm

    Whoever gave you those rules was high. They seem like weird amalgamation of real design principles. First, anyone that makes getters and setters without a need is a bad developer, but that has nothing to do with the concept of getters and setters.
    There is no rule about other classes not knowing internal values of variables, just being able to change the value. That is why it is good practice to make static final vars public.
    Getters and setters can be added to an interface to aid in composition (over inheritance, a real OOP principle)
    Using a getter and setter makes debugging easier since they show up in stack traces. Of coarse you shouldn’t be returning references to collections. That has nothing to do with getters and setters.

    All this is like saying, people use inheritance wrong, therefor it should never be used.

  • 4. Frisian  |  March 1st, 2013 at 3:38 am

    History does repeat itself: http://www.javaworld.com/javaworld/jw-09-2003/jw-0905-toolbox.html
    The usual three-layered approach to application development (presentation layer, business layer, access layer) implies that there won’t be domain objects knowing how to display or store themselves. It’s plain to see, that encapsulation can’t be perfect under these constraints.
    For the access layer it is widely considered acceptable to access the instance variables of domain objects to provide persistence (e.g. Hibernate, Entity Framework or JPA). That is definitely not OO but the alternative (lots of boiler plate code) is even less pleasing.
    Getters and setters are the tools of choice for the presentation layer. The original JavaBeans spec was in fact geared towards UI components. It enables generic treatment of objects by reflection due to a convention on how to expose properties. By and large, it works.
    The business layer usually consists of stateless services, so getters and setters shouldn’t occur at all, them being the epitome of state.
    Beside (and spanning) the layers, there’s the domain model. The domain objects should express business concepts. And I take it, that your example is a domain object.
    The method IsNewBowler() would still be considered a getter in the Java world. So, a getter isn’t bad, when it exposes a business concept. Bowler.IsNew() or Bowler.GetTeam() would be acceptable, too.
    Setters on the other hand are usually bad for two reasons:
    - The property shouldn’t be mutable in the first place, so it should be set in a constructor or a similar construct (factory method, builder etc).
    - It doesn’t express a business concept like “drafted by”, “traded to” or even “fired” (i.e. set Team to null).

    Getters and setters aren’t evil per se. They are tools, and like any tool, should be used where appropriate.

  • 5. Dan Howard  |  March 1st, 2013 at 9:03 am

    There are plenty of situations where you need to control the Object’s state and where you would have a read-only property as a calculated field. Using fields directly prevents this. Now you could take the inconsistent approach and use public fields for “simple” situations and getters/setters for these other cases but that would be er – inconsistent.

  • 6. Jordan  |  March 19th, 2013 at 7:21 am

    I agree, many people misuse getters and setters (or properties in C#). But this does not mean in any way that the are a bad thing per se. Any badly designed method is a bad thing, but this does not mean all methods are bad :)

    The problem is that many people robotically generate public getters and setters for all their private variables, which is wrong.

    The key to getting this stuff right is the core of OO design and encapsulation. When you design a class you should think about the public interface[0]. You should only have a public method when the client code needs it – and a getter or setter is one of those public methods.

    [0] – note that I don’t mean “interface” in terms of the C# or Java key word.

  • 7. Jordan  |  March 19th, 2013 at 7:26 am

    One more point – a mistake I often see people make is to use public setters instead of proper constructors (especially in the world of ORMS – hibernate, entity framework etc)

    If an object needs one of its fields set to function properly, and this field is immutable, then this field should be set via the constructor and *not* by a public property/setter.

Leave a Comment

Required

Required, hidden


five + = 9

Some HTML allowed:
<a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>

Trackback this post  |  Subscribe to the comments via RSS Feed

© 2010-2014 Summa All Rights Reserved