[stellation-res] Database Package Audit.

["Jonathan Gossage" <jgossage@xxxxxxxx>]

Attached is a text file that contains a file by file discussion of all the
points that I noticed in reviewing the database package classes. Enjoy :-)

Regards

Jonathan Gossage

Personal Email
jgossage@xxxxxxxx

Business Email
jonathan@xxxxxxxxxxxxxx

This document  contains a file by file audit of the new database package.


org.eclipse.stellation.repos.database.AbstractDatabase

Javadoc for connectToDatabase

The param "options" is a good example of a method argument whose content cannot be determined from the name and type. In this particular case we have to follow the reference chain back through AbstractBBAccessPoint, where we discover that it is an argument to the constructor named "location". Moving on backward we find in the method workspace.Configure.database that this argument comes from an  option named "location" in a set of options. We also find here that these options are stored in a workspace.Context object which is passed in when the workspace.Configure object is constructed. Again, moving back we can discover that the workspace.Configure object is constructed in the workspace.Svc.command method which receives the context as a parameter. Moving back yet once more we find that workspace.Svc.command is invoked from workspace.Svc.run where we finally discover where the workspace.Context object is created. We also discover that the options are embedded in the field "programArguments which is passed to the workspace.Svc.run method. Moving back one final step we discover that the options came from the arguments passed to main on the command line.

Having got here we at least know where this location option came from but we still do not know what it looks like. It appears we need to go to external documentation to discover this information. The point of this tedious litany is that a substantial amount of work is needed to discover the rules governing the content of the "location" argument. This work can be greatly reduced by following one simple convention.

Use @see with an external hyperlink to link to the section in the document that describes the contents of the argument or use an internal link if the contents of the argument are defined internally within the Javadoc..

I promise I will not be so long winded in future but following this convention will save substantial amounts of time, particularly for newcomers to the code base. The only problem could come when major documentation restructuring breaks the link but a Javadoc audit as part of each major release should deal with this problem.


org.eclipse.stellation.repos.database.AbstractDBAccessPoint

1. The Javadoc for the constructor describes the previous implementation of the constructor. The argument list has changed. Again need a hyperlink for the argument "location".

2. The constructor uses a hard coded string to determine the database type. These strings are scattered throughout the code base in several modules. Should the database names not be stored in String constants so they can be used consistently wherever needed.

3. Also some thought needs to be given to whether knowledge of the specific database in use should be so widely scattered. Ideally all other code should need is an access point. At the moment there is database sensitive code in the following non-database modules:

	org.eclipse.stellation.repos.LocalHandle
	org.eclipse.stellation.workspace.Location
	org.eclipse.stellation.scm.model.ScmAccessSpec
	org.eclipse.stellation.scm.model.ScmEnvirons
	org.eclipse.stellation.repos.artifact.TextArtifact

4. The Javadoc for the createRepository method needs to be updated. The parameter list has changed. Also there should be a hyperlink to a documentation section that discusses the contents of the JDOM document. It is very difficult to keep documentation in sync. if it is duplicated. Since most of the tables are created here, a hyperlink to the document that describes the database tables should be included here.

5. The "createRepository" method contains a startTransaction call but no endTransaction call. Unless special circumstances hold, it is much safer from a code maintenance viewpoint to have both the start and end of a transaction in the same method. When this is not possible, it should be clearly documented at the point where the startTransaction call is made where the endTransaction call will be made.

6. Do you not require Javadoc for getter methods? There is none for "getRepositoryTag".

7. "getAuthenticator" would benefit from a hyperlink to the point where authentication methods are discussed in the documentation.

8. "instantiateAgents" is logging exceptions to stderr. Should it not be using Log4J?

9. "getDatabase" needs a Javadoc entry.

10. "supportsDBType" needs a Javadoc entry.


org.eclipse.stellation.repos.database.BasicDatabase

1. The implementation of "getLongStringType" will generate incorrect code for some database systems unless overridden. For example it could generate VARCHAR(4000) which is illegal in MySql. My feeling is that BasicDatabase should only provide default implementations for types that can be guaranteed to be usable, even if not optimal, on all database systems Accordingly, I would provide no implementation in BasicDatabase for "getStringType" and "getLongStringType".

2. Javadoc entries should be supplied for protected methods since these are used in derived classesand are of interest to developers. The following methods need Javadoc entries:
	renderField
	renderPrimaryKey
	renderForeignKey
	getCreateTableHeader
	getCreateTableTrailer

3. You may want to consider setting auto commit to true in endTransaction. This would provide a safety net that would ensure the integrity of any SQL statements that were inadvertantly not included in a transaction. Alternatively if you want a policy that forces all SQL statements to be run in transactions, you need to add a mechanism to check for this and throw an exception if the contract is violated.

4. So that I understand, why do you use protected fields rather than providing protected accessor functions for derived classes. In general it is best to encapsulate data as fully as possible.


org.eclipse.stellation.repos.database.DB2Database

1. The error text in the exception messages refers to Postgres rather than DB2.


org.eclipse.stellation.repos.database.Table

1. All methods need Javadoc.


org.eclipse.stellation.repos.database.TemplateExpander