getters and setters is all I need

Yes, I have a grudge against hibernate. Yes, hibernate. Oh, it's a great ORM, I use it a lot, it's really ok. But... but if an inexperienced programmer learns from hibernate-style classes then the get/set trouble comes.

Typical "hibernate-class" looks like this (annotations omitted):

public class Client {

private Long id;

private String name;

        private List<String> phones;

// more fields here

public void setId(Long id) {
this.id = id;
}

public Long getId() {
return id;
}

public void setName(String name) {
this.name = name;
}

public String getName() {
return name;
}

public void setPhones(List<String> phones) {
this.phones = phones;
}

        public List<String> getPhones() {
                return phones;
        }

// more getters/setters here

// no other methods - maybe except toString
}

Now, what can you learn from this class, if that's your first meeting with Java ?

Lessons learned from hibernate-class (ironic !):

  • class is only a box for some stuff
  • encapsulation means all the fields are private
  • if there is a field in a class, there must be public get/set method for it
  • get/set always come in pairs - no get without set (and vice versa)
  • get/set does nothing except get/set
  • no need for constructors (beside the default - zero-arg) - get/set will suffice
  • no need for other methods - get/set will suffice

(Two last point sounds like "no need for multiplication, we have addition", lol.)

There are two things I'd like to discuss here. First one is what happens if you use this kind of class with Hibernate, how does it affect the whole system. The second one, is what happens if you apply this style of design to normal classes.

Hibernate

So you have your Client class, it is mapped by hibernate-magic to relational SQL, all goes pretty well. You decide to use Client class not only in DAO layer, but in the whole application (which is quite common), so objects of exactly this class - Client - are traveling to and fro in your application. Let say somewhere in the service layer new phone must be added to this client. How will 99% of developers do it ? Well, it's obvious, isn't it ? That's what getters and setters are exactly for !

client.getPhones().add(phone);

What's wrong here ? Well, the Law of Demeter was broken, which simply means that you'll have more troubles testing this class (you'll need to create two test doubles - for the client and for the list of phones).

Is it as evil as killing small innocent looking, sweet-eyed puppies ? No, it's not so evil. But it's something you shouldn't do.

...you wonder how it should be done ? Ah, it's easy - Client class should have "addPhone" method, or you should use some kind of DTO instead of this class.

Outside Hibernate

And now what happens if a developer believes that get/set is all he needs for his class.

Here comes a simple example, from the code I've seen lately (yeah, it's real).

The purpose of MenuItem class is to hold info about the (suprise, suprise !) menu item, with reference to it's parent (MenuItem also), and it's children (list of MenuItem). It also holds other fields like label, url etc, but it's not important here.

Two things struck me at once.

The way we create MenuItem

Well, it's simple. If MenuItem has no parent:

MenuItem mi = new MenuItem();

If MenuItem has parent:

MenuItem parentMI = ... whatever;
MenuItem mi = new MenuItem();
mi.setParent(parentMI);

Well, that's it. The question is - shouldn't MenuItem have two constructor - zero-arg, creating orphaned MenuItem, and second accepting MenuItem as only parameter setting provided menuItem as it's parent ?

public MenuItem() {
}

public MenuItem(MenuItem parent) {
this();
this.parent = parent;
}

For me, child-parent relationship is rather strong, and if you have parent, you probably have him from the very beginning of your life. So, it makes sense to create this kind of relationship between objects in constructor. IMHO, this:

MenuItem parentMI = ... whatever;
MenuItem mi = new MenuItem(parentMi);

is much better than this:

MenuItem parentMI = ... whatever;
MenuItem mi = new MenuItem();
mi.setParent(parentMI);

The way we set children

Here is a snippet of code I've found somewhere:

MenuItem menuItem = ...whatever;
List<MenuItem> items = new ArrayList<MenuItem>();
for (MenuItem mi : someItems) {
if (someCondition) {
items.add(mi);
}
}
menuItem.setMenuItems(items);

Oh, it's perfect, I mean, it works, well, I mean it works, but if you try to test it you will soon find it gives you a headache... but you won't waste your time writing unit tests, will you ? (ironic !)

...by the way - ever heard of Law of Demeter ?

And what is this intermediary object - items - doing here ? Do we really need this ? Of course not.

IMHO MenuItem should have "addMenuItem" method, and this code should look like this:

MenuItem menuItem = ...whatever;
for (MenuItem mi : someItems) {
if (someCondition) {
menuItem.addMenuItem(mi);
}
}

So what ?

The code works, so what's the big deal ?!

The question is if it really matters. Is it only a matter of style or is it something more, something really important ? Am I just whining or is there a real problem here ?

I'm sure there is. If you don't agree, please read Alen Hollub's Why getter and setter methods are evil article. Maybe it can open your eyes. And do some googling for "immutable objects", please.

I've seen it many times. Classes with no real behavior only get/set methods. It makes other classes to behave in procedural way, cause you can't tell these get/set-classes to do anything useful - you can only take something from them, manipulate it, and give it back. Nothing more. You can't call it OO. The code written in this way soon gets untestable, which means, it's a BAD code. Howgh.

 
 
 
This used to be my blog. I moved to http://tomek.kaczanowscy.pl long time ago.

 
 
 

Please comment using