tags:

views:

175

answers:

4

Hi, as far as I understand, getters/setters should always make copies, in order to protect the data.

However, for many of my classes, it is safe to have the getter return a reference to the property asked for, so that the following code

b = a.getB();
b.setC(someValue);

actually changes the state of object a. If I can prove that it is OK for my class, is it good practice to implement the getter this way? Should the user then be notified of this, for example in the Javadoc? I think that this would break the implementation-hiding paradigm, so, should I always assume that the state of a did not change, and make a call to the setter

b = a.getB();
b.setC(someValue);
a.setB(b);

Thanks in advance S

+4  A: 

Well, the setC violates the Law of Demeter, so I don't think I'd call it a best practice. ("Law" is a bit strong - for instance, it's generally not applied to fluent interfaces.)

That said, getters should not always make copies IMHO. Doing a deep clone can be expensive. There are other options, such as immutable objects.

And, realistically, there are pragmatic considerations.

But I'd err on the side of TMI (too much information) in the JavaDoc.

TrueWill
Thanks for your answer.So stating something like "modifying the returned instance of B will safely modify this object" in the JavaDoc would be acceptable, in your opinion?
Sebastien
@Sebastien: Yes, for a non-copied reference (although I might leave out the "safely"). ;) Personally I'd go with Brian Agnew's answer and use a wrapper object or an immutable one where possible.
TrueWill
+4  A: 

There's a good argument in your above example that since A is maintaining a reference to B, A should look after B, and not hand it out but manipulate it on your behalf. Otherwise you can argue that you're breaking encapsulation (since A reveals it has a reference to B), and ideally objects should do things for you, rather than export their contents such that you can manipulate them.

Having said all that, the above is certainly not an uncommon practise and often a pragmatic choice.

When you expose an object via get(), you have three options:

  1. expose the actual object
  2. make a defensive copy
  3. expose an object that wraps the original, but prohibits modification. e.g. you can wrap the original object in a restricted interface. See (for example) Collections.unmodifiableCollection() which wraps the original collection (and doesn't copy it) but provides an interface that doesn't permit modification.

Whatever you do, you should document it in the interface (and hence in the Javadoc). Otherwise you're at liberty to change it later, and dependent code can easily break.

Brian Agnew
+1 for mentioning the restricted interface. For .NET readers, check out List(T).AsReadOnly and ReadOnlyCollection(T).
TrueWill
I didn't know option 3, and I really (really) like it. Thanks a lot!
Sebastien
I often create a read-only version of a collection and hold onto it as a member variable so I can pass it out in my getters (seems 'better' than re-generating the read-only list over and over).
Kevin Day
A: 

You will get copying wrong before you know it! And then you have a real bug, not a potential one.

Therefore, if you trust the client code, don't bother about it.

This is highly academic and I personally never had a problem with it. I also immediately turn the check off in FindBugs (Java) for example...

And unless there is a problem, who reads JavaDoc and the like anyway? Anybody out there?

raoulsson
I respectfully disagree with everything in this post except for "You will get copying wrong before you know it". The key is **if you trust the client code**. If your method is public on a public class, you generally have no control over future clients.
TrueWill
Who reads Javadoc? - Well I do for one.
Adamski
However, I never had a problem with it. But you do what you have to do, fair enough.
raoulsson
+1  A: 

A further option not yet mentioned is to expose the object via an immutable interface. Obviously this isn't fool-proof as the calling code could always downcast the object into the mutable version, but it avoids any overhead in wrapping the object or creating a copy.

I usually take this approach if I'm writing an API that I'm likely to use myself or within my programming team; i.e. where I know "clients" are going to be good citizens!

// Immutable interface definition.
public interface Record {
  String getContent();
}

// Mutable implementation of Record interface.
public class MutableRecord implements Record {
  private final String content;

  public MutableRecord(String content) {
    this.content = content;
  }

  public String getContent() {
    return content;
  }

  public void setContent(String content) {
    this.content = content;
  }
}

// API that only exposes the object via its Record interface.
public class MyApi {
  private final MutableRecord mutableRecord;

  public Record getRecord() {
    return mutableRecord;
  }
}
Adamski