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();
}

Answer 1

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.

Answer 2

    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.