Code style: use generics everywhere, remove dead code, resource leaks, javadoc etc. #775
Comments
|
Related issue, but different code style aspects: #26 |
|
@bencomp and I discussed this ticket at http://irclog.iq.harvard.edu/dataverse/2014-07-28 and we agreed that this ticket shouldn't be about any particular IDE (Eclipse, Netbeans, etc.). The tool we use should not require an IDE so that we can easily run in from Jenkins. The MongoDB project uses checkstyle, for example (config at https://github.com/mongodb/mongo-java-driver/blob/3.0.x/config/checkstyle.xml ) and FindBugs is also popular. |
|
I just created a draft Dataverse Coding Standards and Style Guide: https://docs.google.com/document/d/1KTd3FpM1BI3HlBofaZjMmBiQEJtFf11jiiGpQeJzy7A/edit?usp=sharing We need to agree on our standards and style before we commit them to the Developers Guide. |
|
Can I add to this that any debug or info messages and stacktraces are not written to |
|
@bencomp this is a good idea and I just added it to the Google doc I mentioned before. I just changed it so the public can add comments and I added you as someone who can edit. I think it'll be easier to have the discussion there. |
|
To add to the list above: unchecked conversions are also needed when |
|
Right, we should be using the createQuery methods on EntityManager that return TypedQuery. |
|
Hi Phil, What is Dataverse's rule about the line-ending style for the GitHub Then, how about a case of modifying an existing file, i.e., allowing a Yesterday when I vagrant-ed up the Dataverse project, it failed because ==> default: /tmp/vagrant-shell: ./download.sh: /bin/sh^M: bad As far as I checked about the Git plugin of NetBeans, there was no On 9/25/2014 10:54 AM, Philip Durbin wrote:
Akio Sone |
|
I just added "Use Unix line endings (LF)" to the doc: On Wed, Nov 5, 2014 at 12:24 PM, Sone, Akio asone@email.unc.edu wrote:
Philip Durbin |
|
On a related note, I just learned of a Java code coverage library called JaCoCo ( http://www.eclemma.org/jacoco/ ) at http://www.programmingthrowdown.com/2015/04/episode-41-nodejs.html that's super easy to use. You just add the following to your
|
|
@pdurbin even red bars in such a report look a lot better than encountering I'm tempted to add "limit length of methods" to the style guide, but I don't know the reasons for creating ~300 line methods |
|
@pdurbin will you add the JaCoCo bit to the pom? |
I'd love to. I think we need to decide where to put the output, however. I consider the output from JaCoCo to be similar to the "maven site" output I played with in the DVN 3.6 days. You can see the output of that experiment at http://dvn.github.io/dvn-mavensitepoc/ ( repo link: https://github.com/dvn/dvn-mavensitepoc ). Along these lines I just pushed 184e687 so this gets updated on every build: https://apitest.dataverse.org/guides/developers/database/schemaspy/relationships.html |
Change INFO messages to WARNINGs when something went wrong; use log message templates instead of concatenation, and the Throwable variant when a Throwable is passed. Improve formatting, add some javadoc.
Instead of using `Object[]` with various types and needing knowledge of the type and meaning of the members of the array, use SummaryData and BlockData. Also, don't use raw types and unchecked conversions.
Manually added diamond operators in files.
Change INFO messages to WARNINGs when something went wrong; use log message templates instead of concatenation, and the Throwable variant when a Throwable is passed. Improve formatting, add some javadoc.
Changes include converting `Query` to `TypedQuery<...>`.
|
In ccea087 I enabled |
|
These issues have been raised internally. Closing and individual issues will be followed up on separately. Closing. |
Add more generics to Guestbook related classes (#775)
As JaCoCo has been a direct dependency in *compile* scope, at four places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
As JaCoCo has been a direct dependency in *compile* scope. At a few places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
As JaCoCo has been a direct dependency in *compile* scope. At a few places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
As JaCoCo has been a direct dependency in *compile* scope. At a few places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
As JaCoCo has been a direct dependency in *compile* scope. At a few places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
As JaCoCo has been a direct dependency in *compile* scope. At a few places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
As JaCoCo has been a direct dependency in *compile* scope. At a few places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
As JaCoCo has been a direct dependency in *compile* scope. At a few places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
As JaCoCo has been a direct dependency in *compile* scope. At a few places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
As JaCoCo has been a direct dependency in *compile* scope. At a few places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
As JaCoCo has been a direct dependency in *compile* scope. At a few places accidentially some classes from transitive dependencies from JaCoCo have been used instead of the classes used commonly within the codebase. Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven. Hopefully this created no harm, but better fix this now.
djbrooke
added this to QA 👍
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
Eclipse reports over 800 warnings in the current Dataverse code. That's a lot better than the 4500+ in recent 3.6.2 code, but too many.
These include
List<Long> listOfLongs = new ArrayList();source == nullafter establishingsourceis of a certain class and unused variables and private fields.secondPassReader.close()statement further down, but when an exception is thrown the JVM may not get there. Is Ingest: CSV ingest gets stuck in "ingest in progress" mode with non-comforming csv file #573 related to this, perhaps?May I also recommend to add a bit of Javadoc to every method longer than, say, 10 lines? It makes understanding the code without looking at each and every line easier.
Bad habit found in v3.6: using a raw Map with 2 values of different classes instead of a new Class with two members, one of each type.
With the proper use of generic types I'm sure you don't crazy stuff like that anymore, do you? ;)
The text was updated successfully, but these errors were encountered: