Code style: use generics everywhere, remove dead code, resource leaks, javadoc etc.

[bencomp]

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

• references to raw types (though most are now half-raw), like List<Long> listOfLongs = new ArrayList();
• dead code like checking whether source == null after establishing source is of a certain class and unused variables and private fields.
• (potential) resource leaks when streams are not closed (though I'm not sure how Java handles these), e.g. secondPassReader in CSVFileReader. There is a 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.

Map someMap = getMap();
TypeOne item1 = (TypeOne) someMap.get(0);
TypeTwo item2 = (TypeTwo) someMap.get(1);

With the proper use of generic types I'm sure you don't crazy stuff like that anymore, do you? ;)

[bencomp]

Related issue, but different code style aspects: #26

[pdurbin]

@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.

[pdurbin]

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.

[bencomp]

Can I add to this that any debug or info messages and stacktraces are not written to System.out, but are logged using a Logger? Looking at System.out might work in unit tests, but in Glassfish's server.log, all these lines are just lines without a source or status. (Using meaningful messages helps, but finding the source of any messages is more helpful to both developers and administrators, I think.)

[pdurbin]

@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.

[bencomp]

To add to the list above: unchecked conversions are also needed when EntityManager.createQuery(String q).getResultsList() is used instead of EntityManager.createQuery(String q, Class<T> c).getResultsList().

[pdurbin]

Right, we should be using the createQuery methods on EntityManager that return TypedQuery.

[akio-sone]

Hi Phil,
one more thing I came across my mind about this topic is the(?)
line-ending style for newly created files for Dataverse.

What is Dataverse's rule about the line-ending style for the GitHub
right now? No rule or just LF (*nix style)?

Then, how about a case of modifying an existing file, i.e., allowing a
developer to reset existing line styles to the official one or keeping
existing lines styles as they are?

Yesterday when I vagrant-ed up the Dataverse project, it failed because
my local copy of a shell script (download.sh) has the CR+LF-(Windows
style ) ending for some reason as shown below:

==> default: /tmp/vagrant-shell: ./download.sh: /bin/sh^M: bad
interpreter: No such file or directory

As far as I checked about the Git plugin of NetBeans, there was no
option to select a line-ending style project wise although adding a
start-up option (e.g., -J-Dline.separator=LF) seems one way to override
NetBeans' default ( use the line-ending style of the platform).

On 9/25/2014 10:54 AM, Philip Durbin wrote:

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.

—
Reply to this email directly or view it on GitHub
#775 (comment).

Akio Sone
Odum Inst.
UNC at Chapel Hill

[akio-sone]

I just added "Use Unix line endings (LF)" to the doc:
https://docs.google.com/document/d/1KTd3FpM1BI3HlBofaZjMmBiQEJtFf11jiiGpQeJzy7A/edit?usp=sharing

On Wed, Nov 5, 2014 at 12:24 PM, Sone, Akio asone@email.unc.edu wrote:

Hi Phil,
one more thing I came across my mind about this topic is the(?)
line-ending style for newly created files for Dataverse.

What is Dataverse's rule about the line-ending style for the GitHub right
now? No rule or just LF (*nix style)?

Then, how about a case of modifying an existing file, i.e., allowing a
developer to reset existing line styles to the official one or keeping
existing lines styles as they are?

Yesterday when I vagrant-ed up the Dataverse project, it failed because my
local copy of a shell script (download.sh) has the CR+LF-(Windows style )
ending for some reason as shown below:

==> default: /tmp/vagrant-shell: ./download.sh: /bin/sh^M: bad
interpreter: No such file or directory

As far as I checked about the Git plugin of NetBeans, there was no option
to select a line-ending style project wise although adding a start-up
option (e.g., -J-Dline.separator=LF) seems one way to override NetBeans'
default ( use the line-ending style of the platform).

On 9/25/2014 10:54 AM, Philip Durbin wrote:

I just created a draft Dataverse Coding Standards and Style Guide:
https://docs.google.com/document/d/1KTd3FpM1BI3HlBofaZjMmBiQEJtFf
11jiiGpQeJzy7A/edit?usp=sharing

We need to agree on our standards and style before we commit them to the
Developers Guide.

—
Reply to this email directly or view it on GitHub
#775 (comment).

Akio Sone
Odum Inst.
UNC at Chapel Hill

Philip Durbin
Software Developer for http://dataverse.org
http://www.iq.harvard.edu/people/philip-durbin

[pdurbin]

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 pom.xml and run mvn package to produce reports like in the screenshot below:

        <dependency>
            <groupId>org.jacoco</groupId>
            <artifactId>jacoco-maven-plugin</artifactId>
            <version>0.7.4.201502262128</version>
        </dependency>
        <dependency>
...
            <plugin>
                <groupId>org.jacoco</groupId>
                <artifactId>jacoco-maven-plugin</artifactId>
                <version>0.7.4.201502262128</version>
                <configuration>
                    <destfile>${basedir}/target/coverage-reports/jacoco-unit.exec</destfile>
                    <datafile>${basedir}/target/coverage-reports/jacoco-unit.exec</datafile>
                </configuration>
                <executions>
                    <execution>
                        <id>jacoco-initialize</id>
                        <goals>
                            <goal>prepare-agent</goal>
                        </goals>
                    </execution>
                    <execution>
                        <id>jacoco-site</id>
                        <phase>package</phase>
                        <goals>
                            <goal>report</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>

[bencomp]

@pdurbin even red bars in such a report look a lot better than encountering // TODO: implement test in a test class. I'd say go ahead 👍

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

[bencomp]

@pdurbin will you add the JaCoCo bit to the pom?

[pdurbin]

@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

[pdurbin]

In ccea087 I enabled mvn -DcompilerArgument=-Xlint:unchecked test on Travis builds at https://travis-ci.org/IQSS/dataverse so now it will be easier to see and link to "unchecked conversion" warnings, as I just did at #2425 (comment)

[kcondon]

These issues have been raised internally. Closing and individual issues will be followed up on separately. Closing.