Is it a bad practice to change the value of an object, bypassing the setter?

Viewed 348

That is, I get the object through the get-method. And then change the value. Thus, I do not create a copy of this object, but change the value of the original.

Example with List:

getMyListFromPojo().addAll(additionalList);

This code will change the value in the field myListFromPojo! Is this a normal practice?

4 Answers

Is it a bad practice to change the value of an object, bypassing the setter?

A good API won't allow you to change the value. getMyListFromPojo() would return an unmodifiable collection, no one from the outside would be able to alter the value returned.

On the other hand, a POJO class is supposed to be a very simple class. I rarely see POJO methods return something more complex (for instance, a copy or an immutable object) than a reference to a corresponding field.

It depends on what you want to achieve with your object, if you need your object to be immutable(except by using some methods provided), then you would need to show a copy of that object(or make it immutable by using methods like Collections.Immutable). To answer your question: it's perfectly fine to modify a list returned by a method(if that is what you want).

This code will change the value in the field myListFromPojo! Is this a normal practice?

It is your code, thus your decision.

If you want to allow for such things, nicely document that in JavaDoc, and ensure that the underlying List is an ArrayList for example that allows any kind of modification.

If, on the other hand, you do not want such changes to take place, then nicely document that in JavaDoc, to then ensure that your APIs do not allow for it to happen. For example by making sure that the List instance is actually some sort of ImmutableList.

And note that both ways can be legit. It is really up to the person defining the APIs here what is acceptable and what not. Of course, the first premise would be that all things in your API behave in the same way.

Beyond that, such DTOs/Beans are typically meant to only contain values, so updating them is often allowed. But of course, it can lead to problems when multiple threads come into play. From a consistency point of view, you are always better off to forbid modifications (instead: changes create new objects with new content). But sometimes the performance penalty for doing that might be considered too high.

I would instead write two methods:

  • one for adding a single object to the list
    public void addToListFromPojo(Object object) {
        this.myListFromPojo.add(object);
    }
  • and another one for adding a list of objects to it
    public void addAllToListOfPojo(List<Object> objectList) {
        this.myListFromPojo.addAll(objectList);
    }

That would result in more (theoretically unnecessary) code, but it would support readability and
keep the responsibility of changing the fields inside the class.

If you don't care for responsibilities of units of your code, then just use less code ==> your example

Related