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

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: