[
https://issues.apache.org/jira/browse/SIS-180?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14171402#comment-14171402
]
M. Le Bihan commented on SIS-180:
---------------------------------
* Please configure your IDE for using spaces rather than tabulations, since
not all editors have the same tabulation width.
Of course !
* Remove the {{public}} modifier for every classes that do not need to be
accessed from outside the package.
Ok.
* Replace star imports by explicit imports, unless there is really a lot of
them (e.g. more than 8 from the same package).
Ok. But I don't like this rules, even if it is common. Stars allow to see
less lines of imports, and avoid folding these imports. When the imports are
fold, people don't check their dependencies and don't detect that their
classes begin to have some links with some unexpected packages.
Most of the time, I am wiping imports from other's code because they are
unused, but they don't know this, because they kept them fold.
* Minor grammatical note: Javadoc convention asks us to use the third person
in the first sentence of method javadoc (e.g. "Format*s*" instead than
"Format").
Ok.
* Some classes have {{\@SuppressWarnings("unused")}} annotation in front of
every arguments, which make method signature very verbose. {{"unused"}} is
not a value recognized by the Oracle {{javac}} compiler (as of JDK8). I
presume that this is an Eclipse-specific one? If this annotation is really
desired, I suggest to put it at the class level at least in {{AbstractFoo}}
classes, for avoiding to repeat it hundred of times.
I didn't know that only eclipse where using the unused SuppressWarnings. I
always set quite all the warnings detection on and take time to resolve all
the troubles that Eclipse detects. Among them, unused variables are to be
checked, because they often show dead code or unexpected actions. So when I
know that a parameter is not used yet (it's not often, its only specific to
the Unimplemented classes that are really special) I put this
@SuppressWarning on the parameters.
* {{unsupportedOperation}} method excepts the source class and method in
argument. But this information is not complete at least in
{{AbstractResultSet}}. We could complete them... but given that this
information duplicates the stack trace information, maybe it could also be
completely omitted.
* It may be worth to take a look at the Javadoc of all JDBC methods and see
if some of them are optional. I suspect that not all methods need to throws
a {{SQLFeatureNotSupportedException}}.
They do not need it. But I want the caller to be immediately stopped by this
exception in order to know which method needs to be implemented quickly. If
I keep void methods (with only logs or nothing), it will become very
difficult to follow what the JDBC classes are willing to do, what methods
are called and when, especially at the time we will create a Datasource and
attempt JPA handle it.
At this time where implementation of the JDBC driver is at its beginning,
its really easy for me to loose myself. I like having stops put everywhere,
in order to discover that I felt in a case I didn't expected.
* In the Javadoc, I guess that all the {{\@see the_implemented_JDBC_method}}
were inserted by Eclipse? Since the Javadoc tool generates this information
automatically in the "Specified by" section, those automatically generated
{{\@see}} tags are redundant and could be omitted.
Ok.
I will be able to do some changes more easily at the time I can commit on
the project.
I've send my icla to the Apache secretary.
Regards,
Marc Le Bihan
-----Message d'origine-----
From: Martin Desruisseaux (JIRA)
Sent: Tuesday, October 14, 2014 6:52 PM
To: [email protected]
Subject: [jira] [Commented] (SIS-180) Place a crude JDBC driver over Dbase
files
[
https://issues.apache.org/jira/browse/SIS-180?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14171208#comment-14171208
]
Martin Desruisseaux commented on SIS-180:
-----------------------------------------
Committed on the JDK8 branch. But before to continue on JDBC support, would
it be possible to do some consolidation work?
I applied the following changes:
* Renamed package as {{org.apache.sis.internal.shapefile.jdbc}}, so those
classes will be hidden from public API.
* Renamed {{XXXUnimplementedFeature}} classes as {{AbstractXXX}}.
* Omitted {{DBaseJDBCDriverUnhandledOperationException}}. Used
{{java.sql.SQLFeatureNotSupportedException}} instead.
* Removed all {{throw new RuntimeException("Should not have been there")}}
lines since they were unnecessary (note: {{throw new AssertionError(...)}}
would have been more accurate). See the code for the simplification trick.
* Defined {{InvalidDbaseFileFormatException}} as a subtype of
{{SQLNonTransientException}} instead than {{SQLException}}.
Would you have a chance to apply the following proposed consolidation, if
you agree?
* Please configure your IDE for using spaces rather than tabulations, since
not all editors have the same tabulation width.
* Remove the {{public}} modifier for every classes that do not need to be
accessed from outside the package.
* Replace star imports by explicit imports, unless there is really a lot of
them (e.g. more than 8 from the same package).
* Minor grammatical note: Javadoc convention asks us to use the third person
in the first sentence of method javadoc (e.g. "Format*s*" instead than
"Format").
* Some classes have {{\@SuppressWarnings("unused")}} annotation in front of
every arguments, which make method signature very verbose. {{"unused"}} is
not a value recognized by the Oracle {{javac}} compiler (as of JDK8). I
presume that this is an Eclipse-specific one? If this annotation is really
desired, I suggest to put it at the class level at least in {{AbstractFoo}}
classes, for avoiding to repeat it hundred of times.
* {{unsupportedOperation}} method excepts the source class and method in
argument. But this information is not complete at least in
{{AbstractResultSet}}. We could complete them... but given that this
information duplicates the stack trace information, maybe it could also be
completely omitted.
* It may be worth to take a look at the Javadoc of all JDBC methods and see
if some of them are optional. I suspect that not all methods need to throws
a {{SQLFeatureNotSupportedException}}.
* In the Javadoc, I guess that all the {{\@see the_implemented_JDBC_method}}
were inserted by Eclipse? Since the Javadoc tool generates this information
automatically in the "Specified by" section, those automatically generated
{{\@see}} tags are redundant and could be omitted.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
> Place a crude JDBC driver over Dbase files
> ------------------------------------------
>
> Key: SIS-180
> URL: https://issues.apache.org/jira/browse/SIS-180
> Project: Spatial Information Systems
> Issue Type: Improvement
> Components: Storage
> Affects Versions: 0.5
> Reporter: M. Le Bihan
> Assignee: Martin Desruisseaux
> Priority: Minor
> Fix For: 0.5
>
> Attachments: src.zip
>
>
> It would be useful to be able to query DBF content through SQL.
> But there are no free drivers available for the old _Dbase 3_ format.
> The first step is to create short implementations of _Connection_,
> _Statement_, _ResultSet_, _ResultSetMetadata_ interfaces for a JDBC using our
> _Database_ class as core binary loader at the begining.
> The main difficulty is to respond to a SQL request, and first : being able to
> analyze it to understand what is expected.
> The SQL request analysis is a very strong job, but I suggest to ease it a lot
> by relying on _AntLR_ API for grammar analysis, associated with a BNF grammar
> file, maybe taken from ^1^ or from elsewhere (grammars are of public domain).
> The goal of this current JIRA is only to be able to perform a
> _SELECT * FROM <shapefile layer name>_
> The WHERE clause or the selection of fields, will come later in other JIRA.
> No transactions, classic _Statement_ only.
> _PreparedStatement_ would be also implemented later (another JIRA).
> Of course, this improvment can be discarded if an open source or free driver
> is discovered, that would allow us to execute SQL requests on DBase 3 easily.
> ^1^ For example, [http://www.savage.net.au/SQL/] has some BNF, but maybe
> elsewhere they will more compliant with AntLR.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
