views:

1097

answers:

8

Do people practically ever use defensive getters/setters? To me, 99% of the time you intend for the object you set in another object to be a copy of the same object reference, and you intend for changes you make to it to also be made in the object it was set in. If you setDate(Date dt) and modify dt later, who cares? Unless I want some basic immutable data bean that just has primitives and maybe something simple like a Date, I never use it.

As far as clone, there are issues as to how deep or shallow the copy is, so it seems kind of "dangerous" to know what is going to come out when you clone an Object. I think I have only used clone() once or twice, and that was to copy the current state of the object because another thread (ie another HTTP request accessing the same object in Session) could be modifying it.

Edit - A comment I made below is more the question: But then again, you DID change the Date, so it's kind of your own fault, hence whole discussion of term "defensive". If it is all application code under your own control among a small to medium group of developers, will just documenting your classes suffice as an alternative to making object copies? Or is this not necessary, since you should always assume something ISN'T copied when calling a setter/getter?

+3  A: 

For both of these issues, the point is explicit control of state. It may be that most of the time you can "get away" without thinking about these things. This tends to be less true as your application gets larger and it gets harder to reason about state and how it propagates among objects.

You've already mentioned a major reason why you'd need to have control over this - being able to use the data safely while another thread was accessing it. It's also easy to make mistakes like this:

class A {
   Map myMap;
}


class B {
   Map myMap;
   public B(A a)
   {
        myMap = A.getMap();//returns ref to A's myMap
   }
    public void process (){ // call this and you inadvertently destroy a
           ... do somethign destructive to the b.myMap... 
     }
}

The point is not that you always want to clone, that would be dumb and expensive. The point is not to make blanket statements about when that sort of thing is appropriate.

Steve B.
Usually getters/setters in API docs are not documented more than "gets this or sets that" so you don't know what it is really doing...
GreenieMeanie
I wouldn't assume, if I were getting a map, that it'd be safe to modify unless the javadoc explicitly stated it was safe. You're seem to be looking for "the" right way - the right way is to be aware of the issue and method side-effects and do what's appropriate.
Steve B.
A good habit is that if you are relying on the state of an object you "Own" (created and contain), Either make it immutable, never return it or return a copy. NEVER TRUST anyone to not break your object, don't give them the ability! (This is also a really good reason to NEVER pass around collections without wrapper classes)
Bill K
Yes, while some defenses are needed, copying something twice should not happen. Unnecessary copying can really hurt the performance.
iny
+7  A: 

From Josh Bloch's Effective Java:

You must program defensively with the assumption that clients of your class will do their best to destroy its invariants. This may actually be true if someone tries to break the security of your system, but more likely your class will have to cope with unexpected behavior resulting from honest mistakes on the part of the programmer using your API. Either way, it is worth taking the time to write classes that are robust in the face of ill-behaved clients.

Item 24: Make defensive copies when needed

bajafresh4life
Yes, that is a good read. In addition - is your object deeply immutable? If so, don't worry about clone().If not, you will wish to override clone to duplicate the members that are mutable.
Sorin Mocanu
I have read that book - good read. However, I am more looking for more real and practical examples. For example, if we peeked inside Java APIs, would any of the classes actually be doing this?
GreenieMeanie
I would hope that the Java API's do this (Josh Block did write a lot of the java.util classes) -- this is really important to do for public API's. Again, most of the time, the object creation overhead involved in making defensive copies is negligible. So if making defensive copies will never really hurt you, but not doing so could introduce REALLY NASTY bugs (trust me, these are bugs of the worst kind) then it's a no brainer.
bajafresh4life
+1  A: 

I've used Clone() to save object state in the user's session to allow for Undo during edits. I've also used it in unit tests.

sal
+2  A: 

This is a non-trivial question. Basically, you have to think about any internal state of a class that you give to any other class via getter or by calling another class' setter. For example, if you do this:

Date now = new Date();
someObject.setDate(now);
// another use of "now" that expects its value to not have changed

then you potentially have two problems:

  1. someObject could potentially change the value of "now", meaning the method above when it later uses that variable could have a different value than it expected, and
  2. if after passing "now" to someObject you change its value, and if someObject did not make a defensive copy, then you've changed the internal state of someObject.

You should either protected against both cases, or you should document your expectation of what is allowed or disallowed, depending on who the client of the code is. Another case is when a class has a Map and you provide a getter for the Map itself. If the Map is part of the internal state of the object and the object expects to fully manage the contents of the Map, then you should never let the Map out. If you must provide a getter for a map, then return Collections.unmodifiableMap(myMap) instead of myMap. Here you probably do not want to make a clone or defensive copy due to the potential cost. By returning your Map wrapped so that it cannot be modified, you are protecting your internal state from being modified by another class.

For many reasons, clone() is often not the right solution. Some better solutions are:

  1. For getters:
    1. Instead of returning a Map, return only Iterators to either the keySet or to the Map.Entry or to whatever allows client code to do what it needs to do. In other words, return something that is essentially a read-only view of your internal state, or
    2. Return your mutable state object wrapped in an immutable wrapper similar to Collections.unmodifiableMap()
    3. Rather than returning a Map, provide a get method that takes a key and returns the corresponding value from the map. If all clients will do with the Map is get values out of it, then don't give clients the Map itself; instead, provide a getter that wraps the Map's get() method.
  2. For constructors:
    1. Use copy constructors in your object constructors to make a copy of anything passed in that is mutable.
    2. Design to take immutable quantities as constructor arguments when you can, rather than mutable quantities. Sometimes it makes sense to take the long returned by new Date().getTime(), for example, rather than a Date object.
    3. Make as much of your state final as possible, but remember that a final object can still be mutable and a final array can still be modified.

In all cases, if there is a question about who "owns" mutable state, document it on the getters or setters or constructors. Document it somewhere.

Here's a trivial example of bad code:

import java.util.Date;

public class Test {
  public static void main(String[] args) {
    Date now = new Date();
    Thread t1 = new Thread(new MyRunnable(now, 500));
    t1.start();
    try { Thread.sleep(250); } catch (InterruptedException e) { }
    now.setTime(new Date().getTime());   // BAD!  Mutating our Date!
    Thread t2 = new Thread(new MyRunnable(now, 500));
    t2.start();
  }

  static public class MyRunnable implements Runnable {
    private final Date date;
    private final int  count;

    public MyRunnable(final Date date, final int count) {
      this.date  = date;
      this.count = count;
    }

    public void run() {
      try { Thread.sleep(count); } catch (InterruptedException e) { }
      long time = new Date().getTime() - date.getTime();
      System.out.println("Runtime = " + time);
    }
  }
}

You should see that each runnable sleeps for 500 msec, but instead you get the wrong time information. If you change the constructor to make a defensive copy:

    public MyRunnable(final Date date, final int count) {
      this.date  = new Date(date.getTime());
      this.count = count;
    }

then you get the correct time information. This is a trivial example. You don't want to have to debug a complicated example.

NOTE: A common result of failure to properly manage state is a ConcurrentModificationException when iterating over a collection.

Should you code defensively? If you can guarantee that the same small team of expert programmers will always be the ones writing and maintaining your project, that they will continuously work on it so they retain memory of the details of the project, that the same people will work on it for the lifetime of the project, and that the project will never become "large," then perhaps you can get away with not doing so. But the cost to defensive programming is not large except in the rarest of cases -- and the benefit is large. Plus: defensive coding is a good habit. You don't want to encourage the development of bad habits of passing mutable data around to places that shouldn't have it. This will bite you some day. Of course, all of this depends on the required uptime of your project.

Eddie
Kind of my whole point (for an example of yours) is begging the question - does it REALLY MATTER if you modify "Date now" later? In other words, will your program crash or will this introduce bugs?
GreenieMeanie
This can definitely introduce bugs.
Eddie
Sure it can. If your setter does any validation, e.g. that the Date is in the past, and then the caller modifies the object to some time in the future, then you've just bypassed a validity check in the object. It's now in an undefined state.
sk
But then again, you DID change the Date, so it's kind of your own fault, hence whole discussion of term "defensive". If it is all application code under your own control among a small to medium group of developers, will just documenting your classes suffice as an alternative to making object copies? Or is this not neccesary, since you should always assume something ISN'T copied when calling a setter/getter?
GreenieMeanie
@GreenieMeanie: It depends on how willing you are to be awakened in the middle of the night when something goes wrong, and how willing you are to spend time debugging something that could have trivially been avoided. It's like defensive driving -- if you get into an accident you could have easily avoided by being defensive, you've just as much lost use of your car as you would have if the accident was your fault.
Eddie
Having programmed for a long time, having worked on many different teams of people with widely varying skill levels, having seen the "demo become the product" more than once, I would always advocate for programming defensively unless the cost of doing so is prohibitive. When the cost is prohibitive, document, document, document. Or check for disallowed state changes in a fail-fast manner the way iterators do in the JDK Collections framework.
Eddie
A: 

I don't like the clone() method, because there is always a type-cast needed. For this reason I use the copy-constructor most of the time. It states more clearly what it does (new object) and you have much control about how it behaves, or how deep the copy is.

At my work we don't worry about defensive programming, although that is a bad habbit. But most of the time it goes ok, but I think I am going to give it a closer look.

Salandur
What if you don't know the class type of the object to be copy/cloned? If I hand you an Animal that you want to copy, which class's copy constructor do you use? Extending from Animal is Mammal, Dog, Cat, etc. The purpose if clone() is that you can get a copy without having to determine its class type.
Steve Kuo
In JDK 1.5+ you can use the clone method and change the return type to your class (although almost no one does because they are used to the old restrictions). Clone is broken for other reasons.
Yishai
A: 

I can think of one situation where clone is much preferable to copy constructors. If you have a function which takes in an object of type X and then returns a modified copy of it, it may be preferable for that copy to be a clone if you want to retain the internal, non-X related information. For example, a function which increments a Date by 5 hours might be useful even if it was passed an object of type SpecialDate. That said, a lot of the time using composition instead of inheritance would avoid such concerns entirely.

Brian
A: 

One thing I'm alwyas missing at a "defensive copy discussion" is the performance aspect. That aiscussion is IMHO a perfect example of performance vs readability/security/robustness.

Defence copies are great for ropbust ode. But if you use it in a time critical part of your app it can be a major performance issue. We had this discussion recently where a data vector stored its data in a double[] values. getValues() returned values.clone(). In our algorithm, getValues() was called for a lot of different objects. When we were wondering, why this simple piece of code took so long to execute, we inspected the code - replaced the return values.clone() with return values and suddenly our total execution time was lowered to less than 1/10 of the original value. Well - I don't need to say that we chose to skip the defensiveness.

Note: I'm not again defensive copies in general. But use your brain when clone()ing!

A: 

I have started using the following practice:

  1. Create copy constructors in your classes but make them protected. The reason for this is that creating objects using the new operator can lead to various issues when working with derived objects.

  2. Create a Copyable interface as follows:

     public interface Copyable<T> {
            public T copy();
     }

Have the copy method of classes implementing Copyable call the protected copy constructor. Derived classes can then call super.Xxx(obj_to_copy); to leverage the base class copy constructor and adding additional functionality as required.

The fact that Java supports covariant return type makes this work. Derived classes simply implement the copy() method as appropriate and return a type-safe value for their particular class.

Frank Hellwig