46

A number of times I've argued that using clone() isn't such a bad practice. Yes, I know the arguments. Bloch said it's bad. He indeed did, but he said that implementing clone() is bad. Using clone on the other hand, especially if it is implemented correctly by a trusted library, such as the JDK, is OK.

Just yesterday I had a discussion about an answer of mine that merely suggests that using clone() for ArrayList is OK (and got no upvotes for that reason, I guess).

If we look at the @author of ArrayList, we can see a familiar name - Josh Bloch. So clone() on ArrayList (and other collections) is perfectly fine (just look at their implementations).

Same goes for Calendar and perhaps most of the java.lang and java.util classes.

So, give me a reason why not to use clone() with JDK classes?

Community
  • 1
  • 1
Bozho
  • 588,226
  • 146
  • 1,060
  • 1,140
  • 3
    Yesterday someone proposed cloning arrays to copy them (instead of System.arraycopy or Arrays.copyOf). Got a +1 from me. http://stackoverflow.com/questions/2589741/how-to-effectively-copy-an-array-in-java/2591414#2591414 – Thilo Apr 08 '10 at 06:29
  • 3
    Remember me when I was telling one of co-worker to NOT add clone method everywhere. But it was difficult to me to explain why :) – Guillaume Oct 07 '10 at 16:22
  • 2
    `clone()` is ok. Many people get it wrong though. I find it great for templates (i.e. factory and class in one - a lot less boilerplate nonsense). Btw, Mr. Bloch books should not be treated as gospel, use your own judgment. A thought exercise: why are not `HashMap.keySet/values` and `ArrayList.subList` serializable? – bestsss May 09 '12 at 09:34

7 Answers7

22

So, give me a reason why not to use clone() with JDK classes?

  • Given an ArrayList reference, you would need a getClass check to check that it is not a subclass of the JDK class. And then what? Potential subclasses cannot be trusted. Presumably a subclass would have different behaviour in some way.
  • It requires that the reference is more specific than List. Some people don't mind that, but the majority opinion is that that is a bad idea.
  • You'll have to deal with a cast, an unsafe cast at that.
Tom Hawtin - tackline
  • 145,806
  • 30
  • 211
  • 305
  • 1
    the cast will be to the type that you call `clone()` on, so it's actually safe (although warnings will appear). But anyway, good points ;) – Bozho Apr 08 '10 at 10:59
  • 4
    A cast is only needed within the clone method. Since Java 1.5 covariant return types are supported, meaning that your clone() method can return any subtype of the return type of the superclass. – helpermethod May 03 '10 at 21:04
  • 4
    @Helper Method: Because of backward compatibility `ArrayList` (and other "collection and JDK classes") do not override `clone` in such a way. – Tom Hawtin - tackline May 03 '10 at 22:21
11

From my experience, the problem of clone() arises on derived classes.

Say, ArrayList implements clone(), which returns an object of ArrayList.

Assume ArrayList has an derived class, namely, MyArrayList. It will be a disaster if MyArrayList does not override the clone() method. (By default it inherits the code from ArrayList).

The user of MyArrayList may expect clone() to return an object of MyArrayList; however, this is not true.

This is annoying: if a base class implements clone(), its derived class has to override the clone() all the way.

SiLent SoNG
  • 4,270
  • 4
  • 27
  • 31
  • 9
    When `ArrayList.clone` is implemented correctly (i.e. it calls `super.clone()` to create the clone instead of instantiating it itself), then `MyArrayList.clone()` **will** return a `MyArrayList` object. There could still be problems with shared references, however. – Joachim Sauer Apr 08 '10 at 07:04
10

I will answer with a quote from the man himself (Josh Bloch on Design - Copy Constructor versus Cloning):

There are very few things for which I use Cloneable anymore. I often provide a public clone method on concrete classes because people expect it.

It can't be any more explicit than this: clone() on his Collection Framework classes are provided because people expect it. If people stop expecting it, he would've gladly thrown it away. One way to get people to stop expecting it is to educate people to stop using it, and not to advocate its use.

Of course, Bloch himself also said (not exact quote but close) "API is like sex: make one mistake and you support it for life". Any public clone() can probably never be taken back. Nevertheless, that's not a good enough reason to use it.

polygenelubricants
  • 376,812
  • 128
  • 561
  • 623
6

In order not to encourage other, less experienced, developers to implement Clone() themselves. I've worked with many developers whose coding styles are largely copied from (sometimes awful) code that they've worked with.

Daniel
  • 10,115
  • 3
  • 44
  • 62
2

It does not enforce whether the implementer will do a deep or shallow copy.

Darin Dimitrov
  • 1,023,142
  • 271
  • 3,287
  • 2,928
  • 5
    it does - (from Object.clone() docs) "Thus, this method performs a "shallow copy" of this object, not a "deep copy" operation.". Anyway, the `ArrayList.clone()` (an the likes) is already implemented and predictable. – Bozho Apr 08 '10 at 08:07
  • the general contract is that `clone()` is shallow - if you intend to perform deep copy you'd be better of w/ a properly named method like `deepCopy` or so. – bestsss May 09 '12 at 09:38
2

Using clone can be risky very risky if you don't check the implementation of this clone method... as you can suppose a clone impl can act in different ways... shallow or deep clone... and some developpers may not always check what kind of clone they will retrieve...

In a big application, big team, it's also risky because when cloning is used, if you modify the clone implementation you may modify the application behaviour and create new bugs... and have to check everywhere the clone was called (but it can be the same for other object methods like equals, toString...

When modifying the clone of a small subclass A (from Deep to Shallow clone for exemple), if an instance of B has a reference to A and a deep clone impl, then since the objects referenced in A are not shallow cloned, the B clone won't be a deep clone anymore (and it's the same for any class referencing a B instance...). It's not easy to deal with the deepness of your clone methods.

Also when you have an interface (extending Clonable) and many (many!) implementations, sometimes you will check some impl and see that clones are deep, and call a clone on the inteface but you can't be sure at runtime all impl really have deep clone and can introduce bugs...

Think it could be better to impl for each method a shallowClone and a deepClone method, and on very specific needs implement customised methods (for exemple you want a clone and limit the depth of this clone to 2, make a custom impl for that in all classes concerned).

Don't think it's a big matter to use a clone method on a JDK class since it's not going to be changed by another developper, or at least not often. But you'd better not call clone on JDK classes if you don't know the class implementation at compile time. I mean calling the clone on an ArrayList is not a matter but calling it on a Collection could be dangerous since another Collection implementation could be introduced by another developper (he can even extends a Collection impl) and nothing tells you that the clone of this impl will work like you expect it to do...

Sebastien Lorber
  • 89,644
  • 67
  • 288
  • 419
0

If the types in your collection are mutable, you have to worry about whether just the collection, itself, will be cloned, or whether the elements will also be cloned... hence implementing your own function, where you know whether the elements or just the container will be cloned will make things much clearer.

Michael Aaron Safyan
  • 93,612
  • 16
  • 138
  • 200