Hello,
I had an issue with switching from copy() (12.X) to clone() (13.X) method with PropagatorBuilders.
Since clone() is not defined at interface level, it relies on Object.clone() which is protected.
So instead of doing:
PropagatorBuilder a;
ProgatorBuilder b = a.clone();
I had to do:
PropagatorBuilder a;
ProgatorBuilder b = ((AbstractPropagatorBuilder) a).clone();
Which I find a bit cumbersome.
I’ve checked and adding a clone() method in the interface PropagatorBuilder fixes the problem and avoids casting.
I’m not very familiar with Java clone philosophy, so what do you think: is it a proper coding rule to add clone() at interface level to make it public or should this be avoided for some reason?
Cheers,
Maxime
Hi Maxime,
In general I’m -0 for using clone() because it creates a new object without using the constructor, which can lead to unexpected behavior because many classes use constructors to enforce invariants.
For example, the constructor of DSSTPropagatorBuilder initialized the field forceModels = new ArrayList(), and getAllForceModels() returns an unmodifiable view of that list, presumably to prevent modification of forceModels out side of that particular instance. Calling clone() would create two instances of DSSTPropagatorBuilder that refer to same list for forceModels so that calling addForceModel(model) on one would also add it to the other. Which I would classify as unexpected behavior.
In this case it’s clear the class hierarchy was not designed to be cloned.
In general I suggest avoiding Cloneable and Serializeable as they both by bypass the constructor when creating new objects, which frequently leads to difficult to diagnose bugs.
Regards,
Evan
2 Likes
Thank you @evan.ward,
So, in your opinion, we shouldn’t have fixed #1378 and kept the “copy()” method. Did I understand well?
My question wasn’t really whether we should use clone or not in the first place, since we already decided to replace “copy” with “clone” a few months ago 
But given what you say, we may want to roll back this change…
Regards,
Maxime
Hi Maxime,
yeah after it was introduced, I noticed my IDE started marking it as a code smell, saying this:
The Object.clone / java.lang.Cloneable mechanism in Java should be considered broken for the following reasons and should, consequently, not be used:
Cloneable is a marker interface without API but with a contract about class behavior that the compiler cannot enforce. This is a bad practice.
Classes are instantiated without calling their constructor, so possible preconditions cannot be enforced.
There are implementation flaws by design when overriding Object.clone, like type casts or the handling of CloneNotSupportedException exceptions.
However, changing the name to copy wouldn’t solve the problem. The IDE suggests a factory or a so-called copy constructor.
Cheers,
Romain.
1 Like