A Bidirectional Add-To-List Mistake

Here’s a somewhat real-world example of a bad coding practice. When setting up bidirectional relationships, its important to remember that the add methods for adding to a list that results in a one-to-many and many-to-one relationship set the parent/child relationship in both directions. To make life simple, I like to provide a couple of different ways of adding to a one-to-many list. For example, say a database contains a “PEOPLE” table made of of Person objects, and each person has many Phones:

@Entity
public class Person {
    @Id
    @GeneratedValue
    private int id;

    private String name;
    private int age;

    @OneToMany(mappedBy = "person")
    @JoinColumn(name = "PERSON_ID")
    private List phones;
    ...
}
@Entity
public class Phone {
    @Id
    @GeneratedValue
    private int id;

    private String number;
    private String type;
    private boolean preferred;

    @ManyToOne(mappedBy = "person")
    @JoinColumn(name="PERSON_ID", nullable=false) 
    private Person person;  
    ...

    public Person getPerson() {
        return person;  
    }
}

Naturally, we need to enforce the bidirectional relation ship like so that adding a phone requires us to add a person (the PERSON_ID foreign key cannot be null!) and adding a phone via a person (person.addPhone(…)) requires that the a phone is added to the phones property and the phone object added to that list is assigned a parent person.

So in our Person class we need a method like this:

addPhone(Phone phone) {
    if (!phones.contains(phone) {
        phones.add(phone);  
        phone.setPerson(this);
    }
}

And in the Phone class we need:

setPerson(Person person) {
    if (this.person != person)
        this.person = person;
}

This is all fine and nice, but sometimes it is much easier to have a method that allows us to set all the child relationships at once. I like to offer both methods (i.e., my Person class would have both an addPhone and addPhoneList method). This isn’t at all uncommon. The problem is that where the phone owner is set may be confusing. Is it inside or outside the POJO?

It is tempting to leave the calling logic to do the work. For example:

List phones = new Phones();
for (some loop) {
    Phone = new phone;
    phones.add(phone);
    phone.setOwner(person);
}

person.addPhoneList(phones);

In our Person class we would then need a method like this:

addPhoneList(List phones) {
    this.phones = phones;
}

This assumes that whoever is writing the calling code understands the need to set the phone owner prior to persisting the person object. It also goes against our previous logic for adding a single phone to the list of phones for a person object. (I realize that I am ignoring the fact that my example method may leave some existing phone numbers orphaned or uni-directional, a separate issue).

A better approach to the addPhoneList method is this:

addPhoneList(List phones) {
    for (Phone phone : phones {
        phone.setPerson(this);
    }
}

By doing things this way we keep the work properly encapsulated to the Person class, and we don’t have to assume that the calling logic (and whoever writes it) set the owner for each phone in the list. Its just cleaner… As with nearly everything, there is a tradeoff between efficiency and bulletproof code. We are forced to loop through the list of phones twice in this scenario: Once when the phones are created and again when inserting a phone list to a Assuming we don’t have a massive list and are not making this call with extreme frequency, the overheard is really negligible.

This seems like a basic problem with an obvious solution, but I have seen code this written this say a number of times (where the add single phone method covers the bidirectional relationship but the add multiple method does not). Hibernate/JPA cannot magically figure out the parent/child relationship, so it is very important to enforce it and properly encapsulate responsibility within the persisted POJOs.

[Level Up: One-To-Many Associations]
[Level Up: Many-To-One Associations]

Advertisements