Running Code Analyzer

[Tom Schindl <to...@gmx.at>]

Hi,

if just installed the TPTP-Plugin and started an analyzing run and it
shows up 7575 more of less questionable results, starting from missing
breaks in switch case block to hardcoded \r\n's.

Should I provide patches to calm down the analyzer because then it would
 may spot better really questionable constructs like the above
mentionned missing breaks.

Tom

Re: Running Code Analyzer

[Jim Vester <ji...@gmail.com>]

FYI,

we recently found an open source  tool called findbugs. It uses BCEL to
perform Byte code analysis.  You may want to run it against the code base.
It is quite good at identifying run time errors and or null pointers etc.

I haven't tried it against ddlutils.

http://findbugs.sourceforge.net/

On 4/20/06, Tom Schindl <to...@gmx.at> wrote:
>
> Tom Schindl wrote:
> > Tom Schindl wrote:
> >
> >>Sometimes code is better so here's a junit-Test to see what I mean.
> >>
> >>Another problem is that I can not run the unit-test directly from within
> >>eclipse because the DTD can not be resolved appropriately the
> >>LocalEntityResolver.resolveEntity() is never called.
> >>
> >>-------------------8<-------------------
> >>SCHWERWIEGEND: Parse Error at line 1 column 23: Document root element
> >>"database", must match DOCTYPE root "null".
> >>org.xml.sax.SAXParseException: Document root element "database", must
> >>match DOCTYPE root "null".
> >>      at
> >>
> com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.createSAXParseException
> (Unknown
> >>Source)
> >>      at
> >>com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.error
> (Unknown
> >>Source)
> >>-------------------8<-------------------
> >>
> >
> >
> > Well that's really interesting if you don't have a <!DOCTYPE>
> > declaration in the source document but set the validation to true the
> > entity resolver doesn't get called and you'll get a parsing exception.
> > All test cases I've looked at don't have a <!DOCTYPE> in them and so the
> > test are failing.
> >
> Sorry they are not failing but you'll only get SAXExceptions all other
> but parsing is not stopped because them.
>
> Tom
>
>
>

Re: Running Code Analyzer

[Tom Schindl <to...@gmx.at>]

Tom Schindl wrote:
> Tom Schindl wrote:
> 
>>Sometimes code is better so here's a junit-Test to see what I mean.
>>
>>Another problem is that I can not run the unit-test directly from within
>>eclipse because the DTD can not be resolved appropriately the
>>LocalEntityResolver.resolveEntity() is never called.
>>
>>-------------------8<-------------------
>>SCHWERWIEGEND: Parse Error at line 1 column 23: Document root element
>>"database", must match DOCTYPE root "null".
>>org.xml.sax.SAXParseException: Document root element "database", must
>>match DOCTYPE root "null".
>>	at
>>com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.createSAXParseException(Unknown
>>Source)
>>	at
>>com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.error(Unknown
>>Source)
>>-------------------8<-------------------
>>
> 
> 
> Well that's really interesting if you don't have a <!DOCTYPE>
> declaration in the source document but set the validation to true the
> entity resolver doesn't get called and you'll get a parsing exception.
> All test cases I've looked at don't have a <!DOCTYPE> in them and so the
> test are failing.
> 
Sorry they are not failing but you'll only get SAXExceptions all other
but parsing is not stopped because them.

Tom

Re: Running Code Analyzer

[Tom Schindl <to...@gmx.at>]

See this simply patch and what's going on with your unit-test. I think
it should be ensured that the result is the same no matter if you are
loading a database-definition file with/without a DTD.

If you agree with me there's no need any more for the test
testColumnWithoutName() because it defaults to the DTD-Default VARCHAR

Tom

Tom Schindl wrote:
> Thomas Dudziak wrote:
> 
>>On 4/21/06, Tom Schindl <to...@gmx.at> wrote:
>>
>>
>>
>>>Well that's really interesting if you don't have a <!DOCTYPE>
>>>declaration in the source document but set the validation to true the
>>>entity resolver doesn't get called and you'll get a parsing exception.
>>>All test cases I've looked at don't have a <!DOCTYPE> in them and so the
>>>test are failing.
>>>
>>>I've now checked:
>>>java-1.4.2_04
>>>java-1.5.2_02
>>>java-1.5.2_06
>>
>>
>>Sure, because how would the XML parser know against which DTD to
>>resolve without the DOCTYPE declaration ?
>>For the test cases we did not put in the DTD because that would only
>>make them slower. For me they are not failing however in Eclipse (not
>>even spitting out errors).
>>
> 
> 
> Although that's reasonable there's a difference when you are loading a
> DTD or not because there are default-values provided by the DTD who are
> merged in automatically by the XML-Parser. Another problem is that even
> your testcases don't follow your own DTD e.g. TestDataReader.java
> defines an attribute defaultValue.
> 
> 
>>Tom
>>
>>
> 
> 

Re: Running Code Analyzer

[Tom Schindl <to...@gmx.at>]

Thomas Dudziak wrote:
> On 4/21/06, Tom Schindl <to...@gmx.at> wrote:
> 
> 
>>Well that's really interesting if you don't have a <!DOCTYPE>
>>declaration in the source document but set the validation to true the
>>entity resolver doesn't get called and you'll get a parsing exception.
>>All test cases I've looked at don't have a <!DOCTYPE> in them and so the
>>test are failing.
>>
>>I've now checked:
>>java-1.4.2_04
>>java-1.5.2_02
>>java-1.5.2_06
> 
> 
> Sure, because how would the XML parser know against which DTD to
> resolve without the DOCTYPE declaration ?
> For the test cases we did not put in the DTD because that would only
> make them slower. For me they are not failing however in Eclipse (not
> even spitting out errors).
> 

Although that's reasonable there's a difference when you are loading a
DTD or not because there are default-values provided by the DTD who are
merged in automatically by the XML-Parser. Another problem is that even
your testcases don't follow your own DTD e.g. TestDataReader.java
defines an attribute defaultValue.

> Tom
> 
> 

Re: Running Code Analyzer

[Thomas Dudziak <to...@gmail.com>]

On 4/21/06, Tom Schindl <to...@gmx.at> wrote:

> Well that's really interesting if you don't have a <!DOCTYPE>
> declaration in the source document but set the validation to true the
> entity resolver doesn't get called and you'll get a parsing exception.
> All test cases I've looked at don't have a <!DOCTYPE> in them and so the
> test are failing.
>
> I've now checked:
> java-1.4.2_04
> java-1.5.2_02
> java-1.5.2_06

Sure, because how would the XML parser know against which DTD to
resolve without the DOCTYPE declaration ?
For the test cases we did not put in the DTD because that would only
make them slower. For me they are not failing however in Eclipse (not
even spitting out errors).

Tom

Re: Running Code Analyzer

[Tom Schindl <to...@gmx.at>]

Tom Schindl wrote:
> Sometimes code is better so here's a junit-Test to see what I mean.
> 
> Another problem is that I can not run the unit-test directly from within
> eclipse because the DTD can not be resolved appropriately the
> LocalEntityResolver.resolveEntity() is never called.
> 
> -------------------8<-------------------
> SCHWERWIEGEND: Parse Error at line 1 column 23: Document root element
> "database", must match DOCTYPE root "null".
> org.xml.sax.SAXParseException: Document root element "database", must
> match DOCTYPE root "null".
> 	at
> com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.createSAXParseException(Unknown
> Source)
> 	at
> com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.error(Unknown
> Source)
> -------------------8<-------------------
> 

Well that's really interesting if you don't have a <!DOCTYPE>
declaration in the source document but set the validation to true the
entity resolver doesn't get called and you'll get a parsing exception.
All test cases I've looked at don't have a <!DOCTYPE> in them and so the
test are failing.

I've now checked:
java-1.4.2_04
java-1.5.2_02
java-1.5.2_06

Mit dem gleichen Ergebnis für alle.

Tom

Re: Running Code Analyzer

[Tom Schindl <to...@gmx.at>]

Sometimes code is better so here's a junit-Test to see what I mean.

Another problem is that I can not run the unit-test directly from within
eclipse because the DTD can not be resolved appropriately the
LocalEntityResolver.resolveEntity() is never called.

-------------------8<-------------------
SCHWERWIEGEND: Parse Error at line 1 column 23: Document root element
"database", must match DOCTYPE root "null".
org.xml.sax.SAXParseException: Document root element "database", must
match DOCTYPE root "null".
	at
com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.createSAXParseException(Unknown
Source)
	at
com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.error(Unknown
Source)
-------------------8<-------------------

Tom

Tom Schindl wrote:
> Thomas Dudziak wrote:
> 
>>On 4/20/06, Tom Schindl <to...@gmx.at> wrote:
>>
>>
>>
>>>yes I understand. Attached is the patch to the questionable missing
>>>break which is when missining intentional strange and should at least be
>>>documented.
>>
>>
>>Thanks, is fixed in SVN
>>
>>
>>
>>>One more question I don't see any code that deals with the situations
>>>where one changes e.g. the name of column/table, removes a column/table,
>>>... which is referenced in indices, foreign keys, ... .
>>
>>
>>I'm not exactly sure what you mean ?
> 
> 
> Well suppose I'm writing a modelling tool based upon your Model-Classes
> having created 2 tables and foreign key constraint. After that I see
> that I have a typo in the name of the the foreign-key provider table, if
> I now rename the table by setting setName() the ForgeinKey-Reference
> doesn't get informed about this and as result the internal structure is
> broken:
> 
> _foreignTableName != _foreignTable.getName()
> 
> if i would now write dump the the files to db.xml I could not load it
> the next time.
> 
> The same applies when renaming columns ;-) I think all this situations
> can be dealt with easily when adding PropertyChangeSupport to all model
> classes and e.g. the foreignKey-Instance adds itself to the
> PropertyChangeListeners and updates its _foreignTableName-Attribute when
> the table name changes.
> 
> Tom
> 
> 
>>Tom
>>
>>
>>
> 
> 

Re: Running Code Analyzer

[Tom Schindl <to...@gmx.at>]

Nevertheless it would be greate if Model-Classes would propagate
PropertyChangeEvents when some internal value changes, wouldn't it?

Not holding duplicate information is one problem which you can address
as you already stated. Another one is what is done if I suddenly remove
a table from the database => foreign key also has to be removed, ... .

Tom


Thomas Dudziak wrote:
> On 4/21/06, Tom Schindl <to...@gmx.at> wrote:
> 
> 
>>Well suppose I'm writing a modelling tool based upon your Model-Classes
>>having created 2 tables and foreign key constraint. After that I see
>>that I have a typo in the name of the the foreign-key provider table, if
>>I now rename the table by setting setName() the ForgeinKey-Reference
>>doesn't get informed about this and as result the internal structure is
>>broken:
>>
>>_foreignTableName != _foreignTable.getName()
>>
>>if i would now write dump the the files to db.xml I could not load it
>>the next time.
>>
>>The same applies when renaming columns ;-) I think all this situations
>>can be dealt with easily when adding PropertyChangeSupport to all model
>>classes and e.g. the foreignKey-Instance adds itself to the
>>PropertyChangeListeners and updates its _foreignTableName-Attribute when
>>the table name changes.
> 
> 
> I can see what you mean, though I probably wouldn't use events for
> this. The major problem is that for parsing from XML it is currently
> necessary to have some duplicated data such as the table names in the
> foreign keys.
> A much better solution would be to retrieve this name of the
> referenced table from the table object directly which would eliminate
> all these problems. Unfortunatley, I havn't had the time yet to
> implement the two-pass parsing algorithm (first only the independent
> items, i.e. the tables/columns, and then the depending items, e.g.
> foreign keys, indexes) necessary for this.
> 
> Tom
> 
> 
> 

Re: Running Code Analyzer

[Thomas Dudziak <to...@gmail.com>]

On 4/21/06, Tom Schindl <to...@gmx.at> wrote:

> Well suppose I'm writing a modelling tool based upon your Model-Classes
> having created 2 tables and foreign key constraint. After that I see
> that I have a typo in the name of the the foreign-key provider table, if
> I now rename the table by setting setName() the ForgeinKey-Reference
> doesn't get informed about this and as result the internal structure is
> broken:
>
> _foreignTableName != _foreignTable.getName()
>
> if i would now write dump the the files to db.xml I could not load it
> the next time.
>
> The same applies when renaming columns ;-) I think all this situations
> can be dealt with easily when adding PropertyChangeSupport to all model
> classes and e.g. the foreignKey-Instance adds itself to the
> PropertyChangeListeners and updates its _foreignTableName-Attribute when
> the table name changes.

I can see what you mean, though I probably wouldn't use events for
this. The major problem is that for parsing from XML it is currently
necessary to have some duplicated data such as the table names in the
foreign keys.
A much better solution would be to retrieve this name of the
referenced table from the table object directly which would eliminate
all these problems. Unfortunatley, I havn't had the time yet to
implement the two-pass parsing algorithm (first only the independent
items, i.e. the tables/columns, and then the depending items, e.g.
foreign keys, indexes) necessary for this.

Tom

Re: Running Code Analyzer

[Tom Schindl <to...@gmx.at>]

Thomas Dudziak wrote:
> On 4/20/06, Tom Schindl <to...@gmx.at> wrote:
> 
> 
>>yes I understand. Attached is the patch to the questionable missing
>>break which is when missining intentional strange and should at least be
>>documented.
> 
> 
> Thanks, is fixed in SVN
> 
> 
>>One more question I don't see any code that deals with the situations
>>where one changes e.g. the name of column/table, removes a column/table,
>>... which is referenced in indices, foreign keys, ... .
> 
> 
> I'm not exactly sure what you mean ?

Well suppose I'm writing a modelling tool based upon your Model-Classes
having created 2 tables and foreign key constraint. After that I see
that I have a typo in the name of the the foreign-key provider table, if
I now rename the table by setting setName() the ForgeinKey-Reference
doesn't get informed about this and as result the internal structure is
broken:

_foreignTableName != _foreignTable.getName()

if i would now write dump the the files to db.xml I could not load it
the next time.

The same applies when renaming columns ;-) I think all this situations
can be dealt with easily when adding PropertyChangeSupport to all model
classes and e.g. the foreignKey-Instance adds itself to the
PropertyChangeListeners and updates its _foreignTableName-Attribute when
the table name changes.

Tom

> 
> Tom
> 
> 
> 

Re: Running Code Analyzer

[Thomas Dudziak <to...@gmail.com>]

On 4/20/06, Tom Schindl <to...@gmx.at> wrote:

> yes I understand. Attached is the patch to the questionable missing
> break which is when missining intentional strange and should at least be
> documented.

Thanks, is fixed in SVN

> One more question I don't see any code that deals with the situations
> where one changes e.g. the name of column/table, removes a column/table,
> ... which is referenced in indices, foreign keys, ... .

I'm not exactly sure what you mean ?

Tom

Re: Running Code Analyzer

[Tom Schindl <to...@gmx.at>]

Hi,

yes I understand. Attached is the patch to the questionable missing
break which is when missining intentional strange and should at least be
documented.

One more question I don't see any code that deals with the situations
where one changes e.g. the name of column/table, removes a column/table,
... which is referenced in indices, foreign keys, ... .

Tom

Thomas Dudziak wrote:
> On 4/20/06, Tom Schindl <to...@gmx.at> wrote:
> 
> 
>>if just installed the TPTP-Plugin and started an analyzing run and it
>>shows up 7575 more of less questionable results, starting from missing
>>breaks in switch case block to hardcoded \r\n's.
>>
>>Should I provide patches to calm down the analyzer because then it would
>> may spot better really questionable constructs like the above
>>mentionned missing breaks.
> 
> 
> You should take these with a big grain of salt. A default
> configuration of PMD would probably show about the same amount of
> 'issues', but a lot of these are either personal style (e.g. number of
> 'duplicated strings' which does not work well when logging is used
> heavily) , or have to be analyzed manually (such as the 'missing
> break' which might or might not be intentional).
> As it is, I'm quite satisified with the code style-wise (not to
> mention that DdlUtils has a configured Checkstyle process which works
> nicely), so I'd rather not start with this.
> 
> Once the 1.0 is out, I probably configure PMD in order to enhance the
> code review process, but currently IMO it would be way more beneficial
> to finish the 1.0 which only misses the revised alteration algorithm
> that I'm currently working on, and a few of the issues that make sense
> for the 1.0.
> 
> cheers,
> Tom
> 
> 

Re: Running Code Analyzer

[Thomas Dudziak <to...@gmail.com>]

On 4/20/06, Tom Schindl <to...@gmx.at> wrote:

> if just installed the TPTP-Plugin and started an analyzing run and it
> shows up 7575 more of less questionable results, starting from missing
> breaks in switch case block to hardcoded \r\n's.
>
> Should I provide patches to calm down the analyzer because then it would
>  may spot better really questionable constructs like the above
> mentionned missing breaks.

You should take these with a big grain of salt. A default
configuration of PMD would probably show about the same amount of
'issues', but a lot of these are either personal style (e.g. number of
'duplicated strings' which does not work well when logging is used
heavily) , or have to be analyzed manually (such as the 'missing
break' which might or might not be intentional).
As it is, I'm quite satisified with the code style-wise (not to
mention that DdlUtils has a configured Checkstyle process which works
nicely), so I'd rather not start with this.

Once the 1.0 is out, I probably configure PMD in order to enhance the
code review process, but currently IMO it would be way more beneficial
to finish the 1.0 which only misses the revised alteration algorithm
that I'm currently working on, and a few of the issues that make sense
for the 1.0.

cheers,
Tom