[vwnc] Code Critic Checks

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[vwnc] Code Critic Checks

Ron Dobbelstein
Hi All,
 
I just downloaded the trial version of WebVelocity and I'm very much impressed so far.
 
Browsing around in the image that comes with it brought the Code Critic Check in the Refactoring Browser under my attention. I know it has been there for ages and my remarks below don't have any relation to WebVelocity (which I think is a really great product), they apply to a "normal" VisualWorks base image as well.
 
If I run the tool at some of the default packages in the base WebVelocity image I get quite a lot of (possible) bugs, e.d.:
  • Base Visualworks: 9478(!) Code Critic Checks of which 238 are bugs and 1486 are possible bugs
  • Glorp: 1269 Code Critic Checks of which 177 are bugs and 163 are possible bugs
  • Seaside: 707 Code Critic Checks of which 28 are bugs and 68 are possible bugs
  • Tools-IDE: 1717 Code Critic Checks of which 35 are bugs and 390 are possible bugs
 
These results made me wonder. Does this mean that the tool is too restrictive, or is there a valid explanation for these (possible) bugs, or are they just plain (possible) bugs? If there is a valid explanation, I think there should be some (documentation) mechanism in place with which one can somehow indicate that one has taken note of the Code Critic Check and the next time the tool is run again, it will ignore the (possible) bug.
 
Does anyone have any thoughts on this?
 
Regards,
 
Ron
 
 
 

De informatie verzonden met dit emailbericht is uitsluitend bestemd voor de geadresseerde. Gebruik van deze informatie door anderen dan de geadresseerde is verboden. Openbaarmaking, vermenigvuldiging, verspreiding en/of verstrekking van deze informatie aan derden is niet toegestaan. Afzender staat niet in voor de juiste en volledige overbrenging van de inhoud van een verzonden email, noch voor tijdige ontvangst daarvan. Afzender attendeert erop dat de vertrouwelijkheid van informatie verzonden per email niet gewaarborgd is.

The information contained in this communication is confidential and may be legally privileged. It is intended solely for the use of the individual or entity to whom it is addressed and others authorised to receive it. If you are not the intended recipient you are hereby notified that any disclosure, copying, distribution or taking any action in reliance on the contents of this information is strictly prohibited and may be unlawful. Sender is neither liable for the proper and complete transmission of the information contained in this communication nor for any delay in its receipt. Please note that the confidentiality of e-mail communication is not warranted.

_______________________________________________
vwnc mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/vwnc
Reply | Threaded
Open this post in threaded view
|

Re: [vwnc] Code Critic Checks

Jan Weerts
Hi Ron,

> These results made me wonder. Does this mean that the tool is
> too restrictive, or is there a valid explanation for these
> (possible) bugs, or are they just plain (possible) bugs? If
> there is a valid explanation, I think there should be some
> (documentation) mechanism in place with which one can somehow
> indicate that one has taken note of the Code Critic Check and
> the next time the tool is run again, it will ignore the
> (possible) bug.

have a look at #lintFilterRule:named:. This can be used in a
Pragma to express, that the developer is aware of lint troubles.
E.g. in our code you will find a lot of
  <lintFilterRule: #'Refactory.Browser.BlockLintRule'
            named: #referenceNonPrerequisiteClass>
in methods to calm down lint, when we do stuff like
  #{Smalltalk.MyClass} valueOrDo: [ ^ nil ].
There are a lot of more methods on the class side of BlockLintRule
to put a diaper on lint.

The results make on the Base make me wonder too sometimes :)

Regards
  Jan

_______________________________________________
vwnc mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/vwnc
Reply | Threaded
Open this post in threaded view
|

Re: [vwnc] Code Critic Checks

Jan Weerts
oops, need to reply to myself:

Jan Weerts wrote:
> have a look at #lintFilterRule:named:. This can be used in a
> Pragma to express, that the developer is aware of lint troubles.

yeah, but this is a local package, which is so old, I forgot it
is not part of any package in the OR.

Sorry
  Jan

_______________________________________________
vwnc mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/vwnc
Reply | Threaded
Open this post in threaded view
|

Re: [vwnc] Code Critic Checks

Holger Guhl
In reply to this post by Jan Weerts
Interesting. Where do you get those filters from? Did you load a special
parcel? I cannot find the #lintFilterRule:named: pragma in plain
VisualWorks image.

Holger Guhl
--
Senior Consultant * Certified Scrum Master * [hidden email]
Tel: +49 231 9 75 99 21 * Fax: +49 231 9 75 99 20
Georg Heeg eK Dortmund
Handelsregister: Amtsgericht Dortmund  A 12812


Jan Weerts schrieb:

> Hi Ron,
>
>  
>> These results made me wonder. Does this mean that the tool is
>> too restrictive, or is there a valid explanation for these
>> (possible) bugs, or are they just plain (possible) bugs? If
>> there is a valid explanation, I think there should be some
>> (documentation) mechanism in place with which one can somehow
>> indicate that one has taken note of the Code Critic Check and
>> the next time the tool is run again, it will ignore the
>> (possible) bug.
>>    
>
> have a look at #lintFilterRule:named:. This can be used in a
> Pragma to express, that the developer is aware of lint troubles.
> E.g. in our code you will find a lot of
>   <lintFilterRule: #'Refactory.Browser.BlockLintRule'
>             named: #referenceNonPrerequisiteClass>
> in methods to calm down lint, when we do stuff like
>   #{Smalltalk.MyClass} valueOrDo: [ ^ nil ].
> There are a lot of more methods on the class side of BlockLintRule
> to put a diaper on lint.
>
> The results make on the Base make me wonder too sometimes :)
>
> Regards
>   Jan
>
> _______________________________________________
> vwnc mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/vwnc
>  
_______________________________________________
vwnc mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/vwnc
Reply | Threaded
Open this post in threaded view
|

Re: [vwnc] Code Critic Checks

Holger Guhl
In reply to this post by Ron Dobbelstein
Hi Ron,
there is a live discussion on what should be reported by Code Critics and how.
There are developers who regard false positives as unacceptable. I have a more pragmatic standpoint, even after developing a tool which collects and reports Code Critics results for software assessment.
Code Critics rules are designed to bring "strange" things in your code to your developer's attention. And it's up to you to decide what's problematic. I am convinced that an automated evaluation procedure can support you but cannot decide what's wrong or good. Especially in software where creativity rules, there are more than one pattern to do the right thing.
I would recommend: Run Code Critics frequently and record the numbers for each version of your released versions. In the long run, this will give an idea how your applications improve or degrade wrt. quality. During development, best before publishing or reviewing, run Code Critics for certain parts of the software. This keeps possible messages within agreeable frame.

Some comments on the numbers you found. I made a similar observation: 8682 rule violations within Base VisualWorks. But you have to interpret. The top scorers which make alone a total of 8073 messages are:

implementedNotSent 2677
typical for a framework: offers service but does not use it
consistencyCheck 2626
uses "size = 0", ... instead of "isEmpty",  "isNil", or "first"
allowed for a framework
refersToClass 379

missingYourself 362
just a warning, most often not a bug
assignmentInIfTrue 343
intention revealing; I like it but it's matter of taste
guardingClause 335
just a pattern; I like it, others don't
tempsReadBeforeWritten 272
just a warning; score relatively small wrt. number of temp.vars
classNameInSelector 169
allowed for a framework
equivalentSuperclassMethods 156
deserves to be cleaned up
ifTrueReturns 120

sendsDifferentSuper 115

toDo 110

sentNotImplemented 109
often guarded by #respondsTo: or reported for superclass sending a selector defined by all subclasses (matter of "missing" #subclassResponsility definition)
searchingLiteral 102

collectionProtocol 99

equalsTrue 99


I do not want to discuss all of them. But the preference of Code Critics is to be strict because this is easier to implement and gives you more encouragement to improve code.
I have seen that running Code Critics and cleaning up code pays off. I found quite a couple of bugs and bad fragments this way.
Cheers
Holger Guhl
-- 
Senior Consultant * Certified Scrum Master * [hidden email]
Tel: +49 231 9 75 99 21 * Fax: +49 231 9 75 99 20
Georg Heeg eK Dortmund
Handelsregister: Amtsgericht Dortmund  A 12812

Ron Dobbelstein schrieb:
Hi All,
 
I just downloaded the trial version of WebVelocity and I'm very much impressed so far.
 
Browsing around in the image that comes with it brought the Code Critic Check in the Refactoring Browser under my attention. I know it has been there for ages and my remarks below don't have any relation to WebVelocity (which I think is a really great product), they apply to a "normal" VisualWorks base image as well.
 
If I run the tool at some of the default packages in the base WebVelocity image I get quite a lot of (possible) bugs, e.d.:
  • Base Visualworks: 9478(!) Code Critic Checks of which 238 are bugs and 1486 are possible bugs
  • Glorp: 1269 Code Critic Checks of which 177 are bugs and 163 are possible bugs
  • Seaside: 707 Code Critic Checks of which 28 are bugs and 68 are possible bugs
  • Tools-IDE: 1717 Code Critic Checks of which 35 are bugs and 390 are possible bugs
 
These results made me wonder. Does this mean that the tool is too restrictive, or is there a valid explanation for these (possible) bugs, or are they just plain (possible) bugs? If there is a valid explanation, I think there should be some (documentation) mechanism in place with which one can somehow indicate that one has taken note of the Code Critic Check and the next time the tool is run again, it will ignore the (possible) bug.
 
Does anyone have any thoughts on this?
 
Regards,
 
Ron
 
 
 

De informatie verzonden met dit emailbericht is uitsluitend bestemd voor de geadresseerde. Gebruik van deze informatie door anderen dan de geadresseerde is verboden. Openbaarmaking, vermenigvuldiging, verspreiding en/of verstrekking van deze informatie aan derden is niet toegestaan. Afzender staat niet in voor de juiste en volledige overbrenging van de inhoud van een verzonden email, noch voor tijdige ontvangst daarvan. Afzender attendeert erop dat de vertrouwelijkheid van informatie verzonden per email niet gewaarborgd is.

The information contained in this communication is confidential and may be legally privileged. It is intended solely for the use of the individual or entity to whom it is addressed and others authorised to receive it. If you are not the intended recipient you are hereby notified that any disclosure, copying, distribution or taking any action in reliance on the contents of this information is strictly prohibited and may be unlawful. Sender is neither liable for the proper and complete transmission of the information contained in this communication nor for any delay in its receipt. Please note that the confidentiality of e-mail communication is not warranted.

_______________________________________________ vwnc mailing list [hidden email] http://lists.cs.uiuc.edu/mailman/listinfo/vwnc


_______________________________________________
vwnc mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/vwnc
Reply | Threaded
Open this post in threaded view
|

Re: [vwnc] Code Critic Checks

Runar Jordahl
We run the code critic rules from unit test classes. We include code
critic rules that we feel we must not create any warnings, and then
run each from a single test method. Below is an example:

testLintRuleBugUndeclaredReference
  self runRule: Refactory.Browser.BlockLintRule undeclaredReference


We have method #runRule: and two other support methods implemented as:

runRule: aLintRule
        Refactory.Browser.SmalllintChecker new
                rule: aLintRule;
                environment: self pundleEnvironment;
                run.
        "Evaluate to bring up methods in a browser: aLintRule problemCount
isZero ifFalse: [aLintRule openEditor]."
        self assert: aLintRule problemCount isZero

pundleEnvironment
        ^Refactory.Browser.PundleEnvironment
                onEnvironment: Refactory.Browser.BrowserEnvironment new
                pundles: (List with: self projectTopBundle)

projectTopBundle
        ^Store.Registry bundleNamed: 'name of our top bundle'


Running code critic is time-consuming, so we store these unit tests in
separate packages/bundles. In these packages we also have unit tests
for running automated UI tests, database tests and spell checking
tests.
       
Currently we run these VW 7.6 rules:
Refactory.Browser.ParseTreeLintRule booleanPrecedence
Refactory.Browser.BlockLintRule overridesSpecialMessage
Refactory.Browser.BlockLintRule undeclaredReference
Refactory.Browser.ParseTreeLintRule collectSelectNotUsed
Refactory.Browser.ParseTreeLintRule indexOf
Refactory.Browser.ParseTreeLintRule sizeCheck
Refactory.Browser.BlockLintRule collectionCopyEmpty
Refactory.Browser.BlockLintRule literalArrayContainsComma
Refactory.Browser.ParseTreeLintRule modifiesCollection
Refactory.Browser.ParseTreeLintRule returnInEnsure
Refactory.Browser.BlockLintRule tempVarOverridesInstVar
Refactory.Browser.ParseTreeLintRule threeElementPoint
Refactory.Browser.BlockLintRule usesTrue
Refactory.Browser.BlockLintRule variableNotDefined
Refactory.Browser.ParseTreeLintRule extraBlock
Refactory.Browser.ParseTreeLintRule asOrderedCollectionNotNeeded

Runar Jordahl
_______________________________________________
vwnc mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/vwnc