mbedward / jaitools

Raster image processing for Java developers
17 stars 15 forks source link

Allow overriding Range implementation #224

Open jdries opened 10 years ago

jdries commented 10 years ago

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!

mbedward commented 10 years ago

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.

jdries commented 10 years ago

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.