Mutable objects - setters and getters

Question

Is it a good practice to have a setter method of this kind? With primitive types, it's obviously fine, but when you have a setter for a field which holds a reference to mutable object, this might go wrong - the caller could modify the object after passing it to setField method.

class Myclass{
   private MyOtherClass field;

   public setField(MyOtherClass field){
      this.field = field;
   }
}

Is it the right thing to simply call clone() (assuming MyOtherClass has only primitive type fields)?

public setField(MyOtherClass field) {
    this.field = field.clone();
}

It is wholly dependent upon what you are trying to accomplish, there are use cases for both ways. FYI, java's default `object.clone()` method does a shallow copy, so, in summary, the former will be susceptible to changes in the object, while your latter example is immune to changes to the object itself, but susceptible to changes in objects referred to by the clone. — Erik Eidt, Mar 31 '16 at 18:36
'susceptible to changes in objects referred to by the clone' - yes, thats why I said assuming the fields in `MyOtherClass` are of primitive type. Otherwise I'd need a copy constructor. — user4205580, Mar 31 '16 at 19:04
Make `MyOtherClass` immutable. The only reason not to is performance. In such case, copying it is also out of the question and you will just have to expose internals of your class. — Banthar, Mar 31 '16 at 20:21
@Banthar what I mean is: suppose there's a private `int[] myField` in my class with a setter. The caller might set the array (with the setter) and then modfy the array in their code. Cloning the array on `set` eliminates the problem. It's not that I want to **assign** to a field only once (and final would make sense then). — user4205580, Mar 31 '16 at 22:01
It's never a good practice to have a setter (but sometimes you can't do otherwise), because with each setter comes a side-effect. And side-effect is what causes a lot of bugs and unpredictability in softwares. — Spotted, Apr 01 '16 at 05:51
Setters and for setting the state of an object. Who says they are for primitive members? — Tulains Córdova, Apr 06 '16 at 21:00
"...with each setter comes a side-effect." Not true. By definition, every setter changes object state, but not every state change is a side-effect. — Syndog, Mar 27 '18 at 14:40

Fermin Silva · Answer 1 · 2016-03-31T19:26:11.363

If your class is a data structure, then obviously it's fine to do so. Java Map has a put method, which stores a reference. If you modify that reference, you would expect to get the modified object back when calling Map.get().

If your class is a Car, and you are setting the Tires, then well, the problem is there: what happens if I set the Tire and then in another part of the program I modify it? In this case (when there are side-effects), if the result is inconsistency, then no, the setter shouldn't be there.

I think what you ask is a bit broad, as there are cases where you should be cautious and others where it's perfectly valid. Setters of that type exist and are all over the place in Java.

Perhaps a better solution is to pass in a class that is immutable (all it's fields are final) if you want to avoid side effects. If I were using an API, I wouldn't expect all it's setters cloning my objects!

Example:

you have a Person class. He can have a JobPlace or not. Do you know the job a person is going to have when you create it? Most likely no. Also, can the person change jobs? Sure.

This justifies a person.setJob(job). Now the person has a reference to his job, but what happens if the job itself changes? The company could change name for example. Person only needs to know the interface of JobPlace to function, so the job itself can change and if several Person's hold the same reference to JobPlace, then you need to change it only in one place.

When it comes to arrays, people suggest cloning it first in the setter. http://stackoverflow.com/questions/6665150/getters-and-setters-for-arrays Then the question is - when to clone objects? — user4205580, Mar 31 '16 at 19:11
In general I wouldn't have a setter and getter for the internal data structures of my classes, as that is exactly what you want to have hidden/encapsulated. I have added another example in the original post — Fermin Silva, Mar 31 '16 at 19:20
See http://programmers.stackexchange.com/a/21809/51711 for more info — Fermin Silva, Mar 31 '16 at 19:27
Yes, but my array is not an internal data structure - it's a result of some calculations and I've chosen to storem them in an array for performance reasons. The class exists for exactly this purpose - to be able to store the result array and some additional information. — user4205580, Mar 31 '16 at 19:30

score -2 · Answer 2 · edited Apr 06 '16 at 20:20

    class Myclass{
       private MyOtherClass field;

this is the variable you first initialize as. In this case, field is a private variable of type MyOtherClass (this is the type you created, hence this is NOT a primitive type). Since you did not set field equal to a final variable, you can set it equal to anything you like, and to do this you can use a constructor. Hence, you use "this.field = field". You can also do this:

 public Myclass(MyOtherClass football)
 {
     field = football;
 }

field is your variable you created and from that point on, you set field equal to your parameter. Think of this as being no different from this.field = field (there is a difference, but let's take one step at a time and understand this first, as to avoid confusion).

so you can use this:

class MyClass{
    private MyOtherClass field;

    public Myclass(MyOtherClass field)
    {
        this.field = field;
    }
}

Or you can use this:

class MyClass{
    private MyOtherClass field;

    public Myclass(MyOtherClass football)
    {
        field = football;
    }
}

And DO NOT clone like that. That is not what clone means (the way you are using clone can cause further confusion and problems later on in your coding projects). Please take some time to read about clone.

This doesn't answer the question. Your two alternatives are only cosmetically different. I think the OP is talking about defensive copying, so that if `MyOtherClass` is mutable, the client code cannot pass it to an instance of `MyClass` (either with a setter or the constructor, like in your example), and *then* mutate outside said instance. — Andres F., Apr 06 '16 at 23:24
(I agree that `clone` is generally broken, which is why [Effective Java](http://www.amazon.com/Effective-Java-Edition-Joshua-Bloch/dp/0321356683) recommends not using it). — Andres F., Apr 06 '16 at 23:26

Mutable objects - setters and getters

2 Answers2