GeoTools

Isolate CanonicalSet as a subclass of WeakHashSet

Details

  • Type: Sub-task Sub-task
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 2.4-M3
  • Fix Version/s: 2.4-M4
  • Component/s: referencing
  • Labels:
    None

Description

WeakHashSet is currently pulling double duty:

  • It functions as a normal set that happens to store its entries using WeakReferences
  • it has additional methods (ie canonicalize) in order to function in a manner similar to String.intern

We should seperate these two concerns - primarily so what is going on can be understood.

  • WeakHashSet - remains as set of weak references, methods providing additional functionality will be deprecated
  • CanonicalSet cerated as an extension to WeakHashSet and proivdes a unique method

This bug report is done when we have updated the code and provided user docs on the result.

Sample use:

class FooFactory {
    public Foo create(String definition) {
          Foo created = new Foo(definition);
          return (Foo) canionicalSet.unique(created);
    }
}

Issue Links

is related to

Sub-task - The sub-task of the issue GEOT-1301 GeoToolsFactory renamed to ReferencingObjectFactory

  • Major - Major loss of function.
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jody Garnett added a comment -
Test case also renamed (had a little bit of fun figuring out the "-ea" VM argument so AllTests could pass - it tests to see if asserts are enabled).
Show
Jody Garnett added a comment - Test case also renamed (had a little bit of fun figuring out the "-ea" VM argument so AllTests could pass - it tests to see if asserts are enabled).
Hide
Permalink
Martin Desruisseaux added a comment -
I'm unsure about this name change. This is true that WeakHashSet is used almost exclusively in order to canonicalize objects, so "CanonicalSet" make that obvious. But collection naming usage at least in J2SE is:

    "Classes that implement the collection interfaces typically have names of the form <Implementation-style><Interface>."
    Source: http://java.sun.com/javase/6/docs/technotes/guides/collections/overview.html

where "implementation-style" is not "the usage for which we think users will want this class".

"WeakHashSet" name was consistent with J2SE "WeakHashMap". "CanonicalSet" has no equivalent in the java.util naming scheme, since it more looks like a "<usage><interface>" pattern.

A quick search show that "WeakHashSet" is a common name for a class doing exactly what the Geotools class does (http://mobius.inria.fr/eclipse-doc/org/eclipse/jdt/internal/compiler/util/WeakHashSet.html, http://adiron.com/AdironORB/1.0.0/doc/javadoc-tools/com/adiron/orb/tools/WeakHashSet.html, etc.). CanonicalSet is much less common.
Show
Martin Desruisseaux added a comment - I'm unsure about this name change. This is true that WeakHashSet is used almost exclusively in order to canonicalize objects, so "CanonicalSet" make that obvious. But collection naming usage at least in J2SE is:     "Classes that implement the collection interfaces typically have names of the form <Implementation-style><Interface>."     Source: http://java.sun.com/javase/6/docs/technotes/guides/collections/overview.html where "implementation-style" is not "the usage for which we think users will want this class". "WeakHashSet" name was consistent with J2SE "WeakHashMap". "CanonicalSet" has no equivalent in the java.util naming scheme, since it more looks like a "<usage><interface>" pattern. A quick search show that "WeakHashSet" is a common name for a class doing exactly what the Geotools class does ( http://mobius.inria.fr/eclipse-doc/org/eclipse/jdt/internal/compiler/util/WeakHashSet.html, http://adiron.com/AdironORB/1.0.0/doc/javadoc-tools/com/adiron/orb/tools/WeakHashSet.html, etc.). CanonicalSet is much less common.
Hide
Permalink
Jody Garnett added a comment -
I see - for me WeakHashSet masked what the class was being used for. I got confused (as did my customer) with official java classes such as java.util.WeakHashMap.

I agree that we should *keep* WeakHashSet - strictly as a HashSet that uses weak references.

I would like the reserve some of the operations such as "canaonicalize" for subclasses.
Show
Jody Garnett added a comment - I see - for me WeakHashSet masked what the class was being used for. I got confused (as did my customer) with official java classes such as java.util.WeakHashMap. I agree that we should *keep* WeakHashSet - strictly as a HashSet that uses weak references. I would like the reserve some of the operations such as "canaonicalize" for subclasses.
Hide
Permalink
Jody Garnett added a comment -
This issue is reopened pending Martin's approval.
Show
Jody Garnett added a comment - This issue is reopened pending Martin's approval.
Hide
Permalink
Martin Desruisseaux added a comment -
I agree with keeping WeakHashSet and moving the "canonicalize" method (or whatever it is named) to a CanonicalSet subclass. The only question is: is it worth to add one more class to an already large API for that? If you believe that the conceptual clarification outweight the addition of one more class, I'm okay.
Show
Martin Desruisseaux added a comment - I agree with keeping WeakHashSet and moving the "canonicalize" method (or whatever it is named) to a CanonicalSet subclass. The only question is: is it worth to add one more class to an already large API for that? If you believe that the conceptual clarification outweight the addition of one more class, I'm okay.
Hide
Permalink
Jody Garnett added a comment -
I have started the documentation here:
- http://docs.codehaus.org/display/GEOTDOC/09+Collection+Classes
Show
Jody Garnett added a comment - I have started the documentation here: - http://docs.codehaus.org/display/GEOTDOC/09+Collection+Classes
Hide
Permalink
Jody Garnett added a comment -
Perhaps InternSet would be a better communicate the functionality being captured.
Show
Jody Garnett added a comment - Perhaps InternSet would be a better communicate the functionality being captured.
Hide
Permalink
Martin Desruisseaux added a comment -
InternSet would match String.intern() but I wonder if the later is suffisiently explicit. I have a slight preference for "CanonicalSet", providing that I'm not misunderstanding the meaning of "Canonical".
Show
Martin Desruisseaux added a comment - InternSet would match String.intern() but I wonder if the later is suffisiently explicit. I have a slight preference for "CanonicalSet", providing that I'm not misunderstanding the meaning of "Canonical".
Hide
Permalink
Martin Desruisseaux added a comment -
I deprecated 'get' and 'canonicalize' from WeakHashSet and added 'get' and 'unique' method in CanonicalSet.

An open question is how to call the "canonicalize" / "toUnique" method. The former seems close in spirit to the following:

    "Canonical form is a unique form of whatever it is you're looking at allowing
     comparing for equality of two items from the space by comparing their
     canonical forms for identity. It must be true that every element of the
     domain has a canonical form and that the canonicalization function is
     idempotent (it doesn't change something already in canonical form)."

     source: http://www.stylusstudio.com/xmldev/200307/post30880.html
     see also: http://en.wikipedia.org/wiki/Canonicalization

However I admit that "canonicalize" may be unfamiliar to many users. On the other hand "toFoo()" in J2SE API often means "convert to an object of different nature", for example Object.toString(), Collection.toArray(), File.toURI(), ColorSpace.toRGB(), etc. Maybe a plain "unique" name may be better.
Show
Martin Desruisseaux added a comment - I deprecated 'get' and 'canonicalize' from WeakHashSet and added 'get' and 'unique' method in CanonicalSet. An open question is how to call the "canonicalize" / "toUnique" method. The former seems close in spirit to the following:     "Canonical form is a unique form of whatever it is you're looking at allowing      comparing for equality of two items from the space by comparing their      canonical forms for identity. It must be true that every element of the      domain has a canonical form and that the canonicalization function is      idempotent (it doesn't change something already in canonical form)."      source: http://www.stylusstudio.com/xmldev/200307/post30880.html      see also: http://en.wikipedia.org/wiki/Canonicalization However I admit that "canonicalize" may be unfamiliar to many users. On the other hand "toFoo()" in J2SE API often means "convert to an object of different nature", for example Object.toString(), Collection.toArray(), File.toURI(), ColorSpace.toRGB(), etc. Maybe a plain "unique" name may be better.
Hide
Permalink
Jody Garnett added a comment -
okay - I can to "unqiue" (although 'intern" may make what is going on even more clear ...)
Cheers!
Show
Jody Garnett added a comment - okay - I can to "unqiue" (although 'intern" may make what is going on even more clear ...) Cheers!
Hide
Permalink
Jody Garnett added a comment -
Thanks for fixing this martin. On to the next one.
Show
Jody Garnett added a comment - Thanks for fixing this martin. On to the next one.
Hide
Permalink
Jody Garnett added a comment -
Opps - next time can you fix the docs as well (the page was here - http://docs.codehaus.org/display/GEOTDOC/09+Collection+Classes)
Show
Jody Garnett added a comment - Opps - next time can you fix the docs as well (the page was here - http://docs.codehaus.org/display/GEOTDOC/09+Collection+Classes)

People

  • Assignee:
    Jody Garnett
    Reporter:
    Jody Garnett
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: