On the new AutoChecker classes

[Martin Desruisseaux <ma...@geomatys.fr>]

Hello Marc

Continuing on the API review before release, I have a few concerns about
the new classes added in org.apache.sis.util.logging:

AbstractAutoCheckerList
---------------------
What is the purpose of a class that just extends ArrayList without
adding any functionality? If this class appears as the return type of
some method, then this is a deprecated practice since it ties List<T> to
a particular implementation (ArrayList).


AbstractAutoChecker
------------------
On naming convention, we (JDK, SIS) usually don't put the Abstract
prefix for classes that do not implement an interface. If there is no
interface, then the base class is named as if it was the interface,
since it is the API that users will handle. Examples in the JDK are
java.lang.Number, java.io.InputStream, java.io.Reader, java.nio.Buffer,
java.text.Format, java.lang.reflect.Reference, java.nio.Charset,
java.util.logging.Handler and much more.

AbstractAutoChecker constructor gives an unusual argument to
getLogger(String): it gives the simple class name (e.g. "DBFConnection")
while the convention is rather to use the package name (e.g.
"org.apache.sis.storage.shapefile").

AbstractAutoChecker stores the Logger reference in a field, while the
recommended usage is to store the logger in a static final field. The
reason is that java.util.logging uses weak references for Logger
instances. Consequently if a user configures a logger for a particular
package, all its configuration is lost if there is no permanent strong
reference to that Logger somewhere. Of course we can not use such static
field in AbstractAutoChecker itself, so it has to be in the subclass
(e.g. DBFConnection).

All format methods in AbstractAutoChecker duplicates the
org.apache.sis.resources.IndexResourceBundle methods. If the later does
not suit your needs, we can discuss about that. But I would like to
avoid duplication in SIS.

    Martin

Re: On the new AutoChecker classes

[Marc Le Bihan <ml...@gmail.com>]

Hello,

    Ok, I will do some change on them at the same time I commit the changes 
for the Shapefile class, next week.

Regards,

Marc.

-----Message d'origine----- 
From: Martin Desruisseaux
Sent: Friday, January 02, 2015 9:28 AM
To: Apache SIS
Subject: On the new AutoChecker classes

Hello Marc

Continuing on the API review before release, I have a few concerns about
the new classes added in org.apache.sis.util.logging:

AbstractAutoCheckerList
---------------------
What is the purpose of a class that just extends ArrayList without
adding any functionality? If this class appears as the return type of
some method, then this is a deprecated practice since it ties List<T> to
a particular implementation (ArrayList).


AbstractAutoChecker
------------------
On naming convention, we (JDK, SIS) usually don't put the Abstract
prefix for classes that do not implement an interface. If there is no
interface, then the base class is named as if it was the interface,
since it is the API that users will handle. Examples in the JDK are
java.lang.Number, java.io.InputStream, java.io.Reader, java.nio.Buffer,
java.text.Format, java.lang.reflect.Reference, java.nio.Charset,
java.util.logging.Handler and much more.

AbstractAutoChecker constructor gives an unusual argument to
getLogger(String): it gives the simple class name (e.g. "DBFConnection")
while the convention is rather to use the package name (e.g.
"org.apache.sis.storage.shapefile").

AbstractAutoChecker stores the Logger reference in a field, while the
recommended usage is to store the logger in a static final field. The
reason is that java.util.logging uses weak references for Logger
instances. Consequently if a user configures a logger for a particular
package, all its configuration is lost if there is no permanent strong
reference to that Logger somewhere. Of course we can not use such static
field in AbstractAutoChecker itself, so it has to be in the subclass
(e.g. DBFConnection).

All format methods in AbstractAutoChecker duplicates the
org.apache.sis.resources.IndexResourceBundle methods. If the later does
not suit your needs, we can discuss about that. But I would like to
avoid duplication in SIS.

    Martin