4

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();
}
  • 3
    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
  • In Java, clone is seldom a good idea :) – Andres F. Mar 31 '16 at 19:50
  • 2
    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
3

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.

  • 1
    When it comes to arrays, people suggest cloning it first in the setter. stackoverflow.com/questions/6665150/… 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 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
-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.

  • 1
    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 6 '16 at 23:24
  • (I agree that clone is generally broken, which is why Effective Java recommends not using it). – Andres F. Apr 6 '16 at 23:26
  • 1
    -1. As Andres said, it misses the point of the question. – MichelHenrich Apr 7 '16 at 2:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.