jaitools icon indicating copy to clipboard operation
jaitools copied to clipboard

Allow overriding Range implementation

Open jdries opened this issue 11 years ago • 2 comments

The current implementation uses a copy constructor to create defensive copies of the Range parameter. The side effect of this approach is that the Range class can not be overridden. I want to be able to override it, so I can provide an implementation of 'contains' which is more efficient for my very specific use case (so the performance improvement can't be generalized.)

This patch makes Range cloneable, and uses the clone method instead of a copy constructor, which solves the issue!

jdries avatar Nov 19 '13 08:11 jdries

Thanks for this.

In general I'd prefer to avoid Cloneable and clone since so many developers have strong misgivings about them (e.g. see http://www.artima.com/intv/bloch13.html and lots of discussion on StackOverflow). However, I think being able to sub-class Range would indeed be useful, so perhaps we can accomplish the same thing by adding a copy method (basically equivalent to clone but without Cloneable) or a static factory method.

I'll float this on the user list (https://groups.google.com/forum/#!forum/jai-tools) and get back to you.

mbedward avatar Nov 20 '13 00:11 mbedward

Hi Michael,

thanks for the feedback. This made me look at the Range implementation again, and I noticed that it is in fact an immutable class. Which makes me wonder why you need to do this defensive copying in the first place.

I'm also willing to adapt my patch to use a copy constructor in combination with a copy() method. This does raise the question of whether you want do define your own 'Cloneable' interface and what the semantics of the copy method should be. In The case of Range, it's not that important because it's a class with primitive fields only, so there is not really a difference between deep clone and shallow clone.

PS: I tried posting to the mailing list, but didn't get permission to join yet.

jdries avatar Nov 21 '13 07:11 jdries