Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [triquetrum-dev] FindBugs

I think we can ignore the ErrorCode issue for the time being and move on.

_C

On 1/25/16 3:31 PM, erwindl0 wrote:
ErrorCode is a quite particular thing that benefits from being "enumerator-like".
It's used to manage an application-wide registry of error codes, severities, descriptions, formatted error logging (to allow automated parsing of log files) etc.
I.e. they're not just numeric constants or so.

The goal is also to allow extension modules to add their own instances, and an easy approach for this is to subclass it and to add extra instances there.
I like the enum-like syntax, as this makes it easy to use.

Personally I don't know how to get the same set of features with interfaces...

erwin

Op 25/01/2016 om 23:16 schreef Christopher Brooks:
If an interface would work for ErrorCode, then that would be good.  However, I'd have to look into it. 

FindBugs is picky about some odd things, but I've definitely found bugs that would be hard to find otherwise.  Coverity Scan is also available to us and finds even more bugs.

FindBugs is sometimes wrong.  When it is, I add a comment about why I think it is wrong to the code.

Adding FindBugs was easy, I just added the following to the <plugins>...</plugins> section of the toplevel pom.xml

  <plugin>
          <groupId>org.codehaus.mojo</groupId>
          <artifactId>findbugs-maven-plugin</artifactId>
          <version>3.0.3</version>
          <configuration>
            <effort>Max</effort>
            <findbugsXmlOutput>true</findbugsXmlOutput>
            <failOnError>false</failOnError>
          </configuration>
          <executions>
            <execution>
              <goals>
                <goal>check</goal>
              </goals>
            </execution>
          </executions>
        </plugin>
I'm using the pom-less Tycho builds (https://wiki.eclipse.org/Tycho/Release_Notes/0.24#POM-less_Tycho_builds), so this just worked for Triquetrum.

To test it, run "mvn findbugs:findbugs".

Running "mvn findbugs:gui" brings up the FindBugs gui for each package.

Then I configured Hudson to generate FindBugs reports for and to look for **/findbugsXml.xml files.

I'll work on generating JavaDoc files next, then running doccheck to find problems with the JavaDoc.

_C

On 1/25/16 12:46 PM, Jay Jay Billings wrote:

W.r.t to ErrorCode, personally I always use an interface for this type of thing. Gets around this naming problem and decreases branching on the flip side too.

I need to get ICE building on the CBI and hook FindBugs up to it. We use SonarQube internally but I'd interested in seeing what FindBugs... er... finds.

Jay

On Jan 25, 2016 3:37 PM, "Erwin de Ley" <erwin.de.ley@xxxxxxxxxx> wrote:
I've checked the report and there are definitely a series of warnings that I want to have resolved.

Good idea to add it in our build!

cheers
erwin



Op 25/01/2016 om 21:26 schreef Erwin de Ley:
Hi Christopher,

I agree that we should follow all kinds of good practices, and FindBugs probably has a valid set of them in its default rules.

But... for the ErrorCode naming I'm a bit afraid to end up with many long names for each extension of the base ErrorCode pseudo-enum.
And that this could reduce readability of code i.o. improving it.

E.g. for a typical usage of an extended ErrorCode such as org.eclipse.triquetrum.processing.ErrorCode : in DefaultTaskProcessingBroker there is a line like :

      futResult.completeExceptionally(new ProcessingException(ErrorCode.TASK_UNHANDLED, "No service found for " + task, null));

I think this is quite readable, and better than e.g.

      futResult.completeExceptionally(new ProcessingException(ProcessingErrorCode.TASK_UNHANDLED, "No service found for " + task, null));

We could maybe start using static imports if we go for the specialized names? Then such code would become

    futResult.completeExceptionally(new ProcessingException(TASK_UNHANDLED, "No service found for " + task, null));

Personally I'm not fond of static imports, but I see many of our younger developers using them, so maybe I'm just a bit old-fashioned ;-)
What do you think?

regards
erwin

Op 25/01/2016 om 20:44 schreef Christopher Brooks:
Erwin,
I've set up FindBugs, see https://hudson.eclipse.org/triquetrum/job/triquetrum/findbugs

There are a bunch of warnings like https://hudson.eclipse.org/triquetrum/job/triquetrum/4/findbugsResult/module.232003606/
ErrorCode.java:26, NM_SAME_SIMPLE_NAME_AS_SUPERCLASS, Priority: High

The class name org.eclipse.triquetrum.validation.ErrorCode shadows the simple name of the superclass org.eclipse.triquetrum.ErrorCode

This class has a simple name that is identical to that of its superclass, except that its superclass is in a different package (e.g., alpha.Foo extends beta.Foo). This can be exceptionally confusing, create lots of situations in which you have to look at import statements to resolve references and creates many opportunities to accidently define methods that do not override methods in their superclasses.

One fix would be to rename org.eclipse.triquetrum.validation.ErrorCode to org.eclipse.triquetrum.validation.ErrorCodeValidation or something.

Or, we could ignore it.

I've found that fixing as many FindBugs warnings as possible is really helpful because it makes the actual bugs standout.  The downside of fixing these bugs is that some of the changes are not natural.

What are your thoughts?

_Christopher
-- 
Christopher Brooks, PMP                       University of California
Academic Program Manager & Software Engineer  US Mail: 337 Cory Hall
CHESS/iCyPhy/Ptolemy/TerraSwarm               Berkeley, CA 94720-1774
cxh@xxxxxxxxxxxxxxxxx, 707.332.0670           (Office: 545Q Cory)



_______________________________________________
triquetrum-dev mailing list
triquetrum-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/triquetrum-dev


_______________________________________________
triquetrum-dev mailing list
triquetrum-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/triquetrum-dev



_______________________________________________
triquetrum-dev mailing list
triquetrum-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/triquetrum-dev

-- 
Christopher Brooks, PMP                       University of California
Academic Program Manager & Software Engineer  US Mail: 337 Cory Hall
CHESS/iCyPhy/Ptolemy/TerraSwarm               Berkeley, CA 94720-1774
cxh@xxxxxxxxxxxxxxxxx, 707.332.0670           (Office: 545Q Cory)


_______________________________________________
triquetrum-dev mailing list
triquetrum-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/triquetrum-dev



_______________________________________________
triquetrum-dev mailing list
triquetrum-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/triquetrum-dev

-- 
Christopher Brooks, PMP                       University of California
Academic Program Manager & Software Engineer  US Mail: 337 Cory Hall
CHESS/iCyPhy/Ptolemy/TerraSwarm               Berkeley, CA 94720-1774
cxh@xxxxxxxxxxxxxxxxx, 707.332.0670           (Office: 545Q Cory)

Back to the top