Source code warnings from Eclipse and FindBugs

[Andrew Swan]

Hi Mike,

I don't know if you have run any source code analysis tools over your project, but they can really help to spot both actual and potential bugs. Two that I regularly use are:

* the project-level warnings built into Eclipse, select "Project -> Properties -> Java Compiler -> Errors/Warnings" from the menu and turn on everything you can. I imagine other IDEs have an equivalent feature.
* the excellent FindBugs utility (http://findbugs.sourceforge.net). It has an Eclipse plugin, which is the easiest approach if you're an Eclipse user, or I believe you can also run it from the command line, e.g. as part of an automated build.

Here's a sample of their output when run against the 2.1.0 snapshot of architecture-rules:

Severity and Description    Path    Resource    Location    Creation Time    Id
H B HE: org.seventytwomiles.springframework.core.io.ClassPathResource defines equals and uses Object.hashCode()    architecture-rules/src/main/java/org/seventytwomiles/springframework/core/io    ClassPathResource.java    line 216    1215656010020    2380811
H C EC: Call to equals() comparing different types in com.seventytwomiles.architecturerules.domain.Rule.addViolation(JPackage)    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/domain    Rule.java    line 227    1215656010005    2380809
H C EC: Call to equals() comparing different types in org.seventytwomiles.springframework.util.ResourceUtils.getFile(URL, String)    architecture-rules/src/main/java/org/seventytwomiles/springframework/util    ResourceUtils.java    line 59    1215656010020    2380812
H C GC: java.lang.String is incompatible with expected argument type com.seventytwomiles.architecturerules.domain.JPackage in com.seventytwomiles.architecturerules.domain.Rule.removePackage(String)    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/domain    Rule.java    line 410    1215656010005    2380810
H D DLS: Dead store to personDao in test.com.seventytwomiles.web.spring.PersonRegistrationController.PersonRegistrationController()    architecture-rules/src/test/java/test/com/seventytwomiles/web/spring    PersonRegistrationController.java    line 37    1215656010020    2380814
H D DLS: Dead store to personService in test.com.seventytwomiles.model.Person.Person()    architecture-rules/src/test/java/test/com/seventytwomiles/model    Person.java    line 40    1215656010020    2380813
The field AbstractArchitectureRulesConfigurationTest.log is never read locally    architecture-rules/src/main/java/com/seventytwomiles/architecturerules    AbstractArchitectureRulesConfigurationTest.java    line 49    1215656003442    2380802
The field AbstractConfigurationFactory.log is never read locally    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/configuration    AbstractConfigurationFactory.java    line 45    1215656003411    2380801
The field CyclicDependencyConfiguration.log is never read locally    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/domain    CyclicDependencyConfiguration.java    line 33    1215656003364    2380796
The field DigesterConfigurationFactory.log is never read locally    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/configuration/xml    DigesterConfigurationFactory.java    line 44    1215656003380    2380797
The field PersonDaoImpl.personService is never read locally    architecture-rules/src/test/java/test/com/seventytwomiles/dao/hibernate    PersonDaoImpl.java    line 36    1215656003083    2380780
The field PersonServiceImpl.personDao is never read locally    architecture-rules/src/test/java/test/com/seventytwomiles/services    PersonServiceImpl.java    line 39    1215656003083    2380778
The field SourceDirectory.log is never read locally    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/domain    SourceDirectory.java    line 38    1215656003348    2380789
The field SourcesConfiguration.log is never read locally    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/domain    SourcesConfiguration.java    line 33    1215656003333    2380788
The field UnmodifiableConfiguration.log is never read locally    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/configuration    UnmodifiableConfiguration.java    line 37    1215656003380    2380800
The local variable personDao is never read    architecture-rules/src/test/java/test/com/seventytwomiles/web/spring    PersonRegistrationController.java    line 37    1215656003067    2380777
The local variable personService is never read    architecture-rules/src/test/java/test/com/seventytwomiles/model    Person.java    line 40    1215656003083    2380779
The parameter path is hiding a field from type ClassPathResource    architecture-rules/src/main/java/org/seventytwomiles/springframework/core/io    ClassPathResource.java    line 349    1215656003270    2380784
The parameter path is hiding a field from type SourceDirectory    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/domain    SourceDirectory.java    line 257    1215656003348    2380790
Unnecessary cast from Rule to Rule    architecture-rules/src/test/java/com/seventytwomiles/architecturerules/configuration/xml    DigesterConfigurationFactoryTest.java    line 122    1215656003176    2380781
Unnecessary cast from Rule to Rule    architecture-rules/src/test/java/com/seventytwomiles/architecturerules/configuration/xml    DigesterConfigurationFactoryTest.java    line 160    1215656003176    2380782

I commend these tools to you as a way of rooting out existing bugs and preventing new ones...

Hope this helps,

Andrew

[Mike Nereson]

Thanks. The only two real ones there are

Unnecessary cast from Rule to Rule    architecture-rules/src/test/java/com/seventytwomiles/architecturerules/configuration/xml    DigesterConfigurationFactoryTest.java    line 122   

Unnecessary cast from Rule to Rule    architecture-rules/src/test/java/com/seventytwomiles/architecturerules/configuration/xml    DigesterConfigurationFactoryTest.java    line 160   

The rest are emptyish test files, or .log files that I think one of your tools must have created.

Thanks for passing that along.

~ Mike Nereson

[Andrew Swan]

Indeed most of those look like they can be ignored, but I would have thought these first three would be worth looking into:

Severity and Description    Path    Resource    Location    Creation Time    Id

H C EC: Call to equals() comparing different types in com.seventytwomiles.architecturerules.domain.Rule.addViolation(JPackage)    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/domain    Rule.java    line 227    1215656010005    2380809

H C GC: java.lang.String is incompatible with expected argument type com.seventytwomiles.architecturerules.domain.JPackage in com.seventytwomiles.architecturerules.domain.Rule.removePackage(String)    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/domain    Rule.java    line 410    1215656010005    2380810

The parameter path is hiding a field from type SourceDirectory    architecture-rules/src/main/java/com/seventytwomiles/architecturerules/domain    SourceDirectory.java    line 257    1215656003348    2380790

These next ones aren't from your codebase, but they might be negatively affecting it:

The parameter path is hiding a field from type ClassPathResource    architecture-rules/src/main/java/org/seventytwomiles/springframework/core/io    ClassPathResource.java    line 349    1215656003270    2380784

H C EC: Call to equals() comparing different types in org.seventytwomiles.springframework.util.ResourceUtils.getFile(URL, String)    architecture-rules/src/main/java/org/seventytwomiles/springframework/util    ResourceUtils.java    line 59    1215656010020    2380812

H B HE: org.seventytwomiles.springframework.core.io.ClassPathResource defines equals and uses Object.hashCode()    architecture-rules/src/main/java/org/seventytwomiles/springframework/core/io    ClassPathResource.java    line 216    1215656010020    2380811

Cheers,

Andrew

[Andrew Swan]

... and those ".log" entries aren't log files, they are Apache Commons loggers declared as fields in some of your classes. If I ever want to leave a logger declared in my code even though it's not currently used, I just give it protected access so that FindBugs doesn't warn me about it being unused; this also means any subclasses will inherit it, which is handy.

Cheers,

Andrew

[Mike Nereson]

Thanks explaining logs and for pointing out those first three that I miss. I need to release 2.1.1 tonight. I'll get those fixed in there too.


~ Mike Nereson

[Mike Nereson]

Andrew, I fixed some (many) of the issues for this release tonight, 2.1.1.

If you want to run the analysis on 2.1.1 and if you find more I would love to invite you to help fix some of them. Just create an issue for each type of error, then attach the patch to the resolved issue. I'll apply the patch and commit the changes.

That goes to anyone for any issue. Thanks.

~ Mike Nereson