[#GEOT-1300] Isolate CanonicalSet as a subclass of WeakHashSet - jira.codehaus.org

Jody Garnett made changes - 12/Jun/07 2:19 PM

Field	Original Value	New Value
Status	Open [ 1 ]	In Progress [ 3 ]

Hide

Permalink

Jody Garnett added a comment - 12/Jun/07 2:31 PM

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 - 12/Jun/07 2:31 PM 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).

Jody Garnett made changes - 12/Jun/07 2:31 PM

Status	In Progress [ 3 ]	Closed [ 6 ]
Resolution		Fixed [ 1 ]

Martin Desruisseaux made changes - 13/Jun/07 9:29 AM

Link

This issue is related to ~~GEOT-1301~~ [ ~~GEOT-1301~~ ]

Hide

Permalink

Martin Desruisseaux added a comment - 13/Jun/07 10:01 AM

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 - 13/Jun/07 10:01 AM 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 - 13/Jun/07 11:30 AM

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 - 13/Jun/07 11:30 AM 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 - 13/Jun/07 12:02 PM

This issue is reopened pending Martin's approval.

Show

Jody Garnett added a comment - 13/Jun/07 12:02 PM This issue is reopened pending Martin's approval.

Jody Garnett made changes - 13/Jun/07 12:02 PM

Resolution	Fixed [ 1 ]
Status	Closed [ 6 ]	Reopened [ 4 ]

Hide

Permalink

Martin Desruisseaux added a comment - 13/Jun/07 12:05 PM

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 - 13/Jun/07 12:05 PM 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 - 13/Jun/07 12:08 PM

I have started the documentation here:
- http://docs.codehaus.org/display/GEOTDOC/09+Collection+Classes

Show

Jody Garnett added a comment - 13/Jun/07 12:08 PM I have started the documentation here: - http://docs.codehaus.org/display/GEOTDOC/09+Collection+Classes

Jody Garnett made changes - 13/Jun/07 12:08 PM

Summary	WeakHashSet renamed to CanonicalSet	Isolate CanonicalSet as a subclass of WeakHashSet
Description	WeakHashSet just deprecated for now, simply extends CanonicalSet which is its replacement. We need to rename any associated test case as well.	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 - primarly 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 extention to WeakHashSet and proivdes a toUnique method This bug report is done when we have updated the code and provided user docs on the result. Sample use: {code} class FooFactory { public Foo create(String definition) { Foo created = new Foo(definition); return (Foo) canionicalSet.toUnique(created); } } {code}

Hide

Permalink

Jody Garnett added a comment - 13/Jun/07 12:09 PM

Perhaps InternSet would be a better communicate the functionality being captured.

Show

Jody Garnett added a comment - 13/Jun/07 12:09 PM Perhaps InternSet would be a better communicate the functionality being captured.

Hide

Permalink

Martin Desruisseaux added a comment - 13/Jun/07 12:13 PM

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 - 13/Jun/07 12:13 PM 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 - 14/Jun/07 3:28 AM

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 - 14/Jun/07 3:28 AM 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 - 18/Jun/07 12:17 PM

okay - I can to "unqiue" (although 'intern" may make what is going on even more clear ...)
Cheers!

Show

Jody Garnett added a comment - 18/Jun/07 12:17 PM okay - I can to "unqiue" (although 'intern" may make what is going on even more clear ...) Cheers!

Jody Garnett made changes - 18/Jun/07 12:23 PM

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 - primarly 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 extention to WeakHashSet and proivdes a toUnique method

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

Sample use:
{code}
class FooFactory {
    public Foo create(String definition) {
          Foo created = new Foo(definition);
          return (Foo) canionicalSet.toUnique(created);
    }
}
{code}

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 - primarly 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 extention to WeakHashSet and proivdes a toUnique method

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

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

Jody Garnett made changes - 18/Jun/07 12:24 PM

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 - primarly 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 extention to WeakHashSet and proivdes a toUnique method

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

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

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:
{code}
class FooFactory {
    public Foo create(String definition) {
          Foo created = new Foo(definition);
          return (Foo) canionicalSet.unique(created);
    }
}
{code}

Hide

Permalink

Jody Garnett added a comment - 18/Jun/07 12:25 PM

Thanks for fixing this martin. On to the next one.

Show

Jody Garnett added a comment - 18/Jun/07 12:25 PM Thanks for fixing this martin. On to the next one.

Jody Garnett made changes - 18/Jun/07 12:25 PM

Resolution		Fixed [ 1 ]
Status	Reopened [ 4 ]	Closed [ 6 ]

Hide

Permalink

Jody Garnett added a comment - 18/Jun/07 12:26 PM

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 - 18/Jun/07 12:26 PM Opps - next time can you fix the docs as well (the page was here - http://docs.codehaus.org/display/GEOTDOC/09+Collection+Classes)

Martin Desruisseaux made changes - 07/Jul/07 5:54 AM