[Pharo-project] Feedback about SmallLint
Hi guys. After our meeting at Lille, I did a pass of SmallLint over Fuel project. I have to say that I enjoyed the tool and that. I love the manifiesto idea. I love to store metada and to be able to reject rules and scenarios to avoid having false positives again in the future. In addition, it looks like I had less false positive than with other tools. However, there were a couple of rules that I don't understand or may suggest something: 1) First, apart form showing the package between parenthesis, I would also show the protocol (category). Because some rules play with that, so it is handy if you can directly see them there, otherwise you need to browse the code. 2) RBContainsRule is not clear for me. *" 'Checks for the common code fragment: "(aCollection detect: [:each | ''some condition''] ifNone: [nil]) ~= nil". contains: can simplify this code to "aCollection contains: [:each | ''some condition'']". Not only is the contains: variant shorter, it better signifies what the code is doing.'"* That is not very true. #contains answers true or false, while detect:ifNone:[] answers whatever, and in your example it is nil. So, the answer will be different and hence it is not the same. Ok, in your example, you compare after with == nil... but... I don't really see a use case. For example, in my case it detected: classNamed: className ^ (migrations detect: [:m | m sourceClassName = className ] ifNone: [ ^ self globalClassNamed: className ]) targetClass. 3) RBLawOfDemeterRule: maybe you can exclude string concatenation? (#,) because it detected a lot of messages like this: signalWith: aGlobalName and: aSelector ^ self signal: 'Method ', aGlobalName, '>>#', aSelector, ' not found.' Also, what about excluding messages like #asXXX. For example, here it detected "self signature asByteArray". Is that completly breaking the law of demeter? mmm I don't think so. verifySignatureFrom: aDecoder | streamSignature | streamSignature := ByteArray new: self signature size. aDecoder nextEncodedBytesInto: streamSignature. (self signature asByteArray = streamSignature) ifFalse: [ FLBadSignature signalCurrentSignature: self signature streamSignature: streamSignature ]. 4) RBSentNotImplementedRule: I didn't understand why it has selected: largeIdentityHash <primitive: 75> self primitiveFailed both messages are implemented :( 5) RBClassInstVarNotInitializedRule could be smarter and search not only for #initialize but for #initializeXXX. Lots of times, I initialize my objects with data, using a particular nitialize method, such as: initializeWithBehaviors: someBehaviors extensionMethods: someExtensionMethods so the rule says there is no initialize...but I have that one ;) That's all the feedback so far. It helped me to find some things to improve so thanks! -- Mariano http://marianopeck.wordpress.com
thanks for the report. Mariano so far we did not change any of the rules provided by RB. For now we will run small lint and collect information. Stef
Hi guys. After our meeting at Lille, I did a pass of SmallLint over Fuel project. I have to say that I enjoyed the tool and that. I love the manifiesto idea. I love to store metada and to be able to reject rules and scenarios to avoid having false positives again in the future. In addition, it looks like I had less false positive than with other tools. However, there were a couple of rules that I don't understand or may suggest something:
1) First, apart form showing the package between parenthesis, I would also show the protocol (category). Because some rules play with that, so it is handy if you can directly see them there, otherwise you need to browse the code.
2) RBContainsRule is not clear for me. " 'Checks for the common code fragment: "(aCollection detect: [:each | ''some condition''] ifNone: [nil]) ~= nil". contains: can simplify this code to "aCollection contains: [:each | ''some condition'']". Not only is the contains: variant shorter, it better signifies what the code is doing.'" That is not very true. #contains answers true or false, while detect:ifNone:[] answers whatever, and in your example it is nil. So, the answer will be different and hence it is not the same. Ok, in your example, you compare after with == nil... but... I don't really see a use case. For example, in my case it detected:
classNamed: className
^ (migrations detect: [:m | m sourceClassName = className ] ifNone: [ ^ self globalClassNamed: className ]) targetClass.
3) RBLawOfDemeterRule: maybe you can exclude string concatenation? (#,) because it detected a lot of messages like this: signalWith: aGlobalName and: aSelector ^ self signal: 'Method ', aGlobalName, '>>#', aSelector, ' not found.'
Also, what about excluding messages like #asXXX. For example, here it detected "self signature asByteArray". Is that completly breaking the law of demeter? mmm I don't think so.
verifySignatureFrom: aDecoder
| streamSignature | streamSignature := ByteArray new: self signature size. aDecoder nextEncodedBytesInto: streamSignature. (self signature asByteArray = streamSignature) ifFalse: [ FLBadSignature signalCurrentSignature: self signature streamSignature: streamSignature ].
4) RBSentNotImplementedRule: I didn't understand why it has selected:
largeIdentityHash
<primitive: 75> self primitiveFailed
both messages are implemented :(
5) RBClassInstVarNotInitializedRule could be smarter and search not only for #initialize but for #initializeXXX. Lots of times, I initialize my objects with data, using a particular nitialize method, such as: initializeWithBehaviors: someBehaviors extensionMethods: someExtensionMethods so the rule says there is no initialize...but I have that one ;)
That's all the feedback so far. It helped me to find some things to improve so thanks!
-- Mariano http://marianopeck.wordpress.com
On Sat, Apr 21, 2012 at 5:01 PM, Stéphane Ducasse <stephane.ducasse@inria.fr
wrote:
thanks for the report. Mariano so far we did not change any of the rules provided by RB. For now we will run small lint and collect information.
Ok, I understand. But since I am SmallLintying my packages I guess that my feedback does not hurt for future work (when you do start changing rules). Some more feedback: 6) RBAbstractClassRule is confusing. *"Checks for references to classes that have subclassResponsibility methods. Such references might be creating instances of the abstract class or more commonly being used as the argument to an isKindOf: message which is considered bad style."* In my case, it found FLBenchmarkRunner because it has INSTANCE side messages with subclassResponsability. However, all references to the class FLBenchmarkRunner send class side messages to it. So.... maybe It can exclude class side messages that do not end up sending #new or #basicNew? 7) how does the Manifiesto class envolves while the real packages are changed (class and methods are renamed or removed) ? If a method class is removed then I guess it is easy to remove the metadada associated to them. But what about if a method change? you cannot be sure if the rule exclusion still make sense or not. So, I guess you should remove the metadata as well ? 8) RBMethodHasNoTimeStampRule seems to have a problem with Traits because it shows me lots of methods without timestamp and they all result to come from a trait. 9) Sometimes, clicking on a method raises an error. Find attached PhaorDebug.log 10) RBSubclassResponsibilityNotDefinedRule could show me WHICH are the leaf subclasses that do NOT implement the method ... otherwise it is complicated to find it. 11) I don't like storing the Manifiest class in the default/main package. I changed ManifiestFuel to be in Fuel-SmallLint rather than Fuel but then, as soon as I run the rules again, it moves back the class to Fuel. Can't it check if the class already exist in Smalltalk globals and if true just use it and don't move it ? 12) It would be nice to group rules by criteria. I saw you have #group in each rule so I guess it is a matter of time to have the UI aware of that, right? Cheers
Stef
Hi guys. After our meeting at Lille, I did a pass of SmallLint over Fuel project. I have to say that I enjoyed the tool and that. I love the manifiesto idea. I love to store metada and to be able to reject rules and scenarios to avoid having false positives again in the future. In addition, it looks like I had less false positive than with other tools. However, there were a couple of rules that I don't understand or may suggest something:
1) First, apart form showing the package between parenthesis, I would also show the protocol (category). Because some rules play with that, so it is handy if you can directly see them there, otherwise you need to browse the code.
2) RBContainsRule is not clear for me. " 'Checks for the common code fragment: "(aCollection detect: [:each | ''some condition''] ifNone: [nil]) ~= nil". contains: can simplify this code to "aCollection contains: [:each | ''some condition'']". Not only is the contains: variant shorter, it better signifies what the code is doing.'" That is not very true. #contains answers true or false, while detect:ifNone:[] answers whatever, and in your example it is nil. So, the answer will be different and hence it is not the same. Ok, in your example, you compare after with == nil... but... I don't really see a use case. For example, in my case it detected:
classNamed: className
^ (migrations detect: [:m | m sourceClassName = className ] ifNone: [ ^ self globalClassNamed: className ]) targetClass.
3) RBLawOfDemeterRule: maybe you can exclude string concatenation? (#,) because it detected a lot of messages like this: signalWith: aGlobalName and: aSelector ^ self signal: 'Method ', aGlobalName, '>>#', aSelector, ' not found.'
Also, what about excluding messages like #asXXX. For example, here it detected "self signature asByteArray". Is that completly breaking the law of demeter? mmm I don't think so.
verifySignatureFrom: aDecoder
| streamSignature | streamSignature := ByteArray new: self signature size. aDecoder nextEncodedBytesInto: streamSignature. (self signature asByteArray = streamSignature) ifFalse: [ FLBadSignature signalCurrentSignature: self signature streamSignature: streamSignature ].
4) RBSentNotImplementedRule: I didn't understand why it has selected:
largeIdentityHash
<primitive: 75> self primitiveFailed
both messages are implemented :(
5) RBClassInstVarNotInitializedRule could be smarter and search not only for #initialize but for #initializeXXX. Lots of times, I initialize my objects with data, using a particular nitialize method, such as: initializeWithBehaviors: someBehaviors extensionMethods: someExtensionMethods so the rule says there is no initialize...but I have that one ;)
That's all the feedback so far. It helped me to find some things to improve so thanks!
-- Mariano http://marianopeck.wordpress.com
-- Mariano http://marianopeck.wordpress.com
thanks for the report. Mariano so far we did not change any of the rules provided by RB. For now we will run small lint and collect information.
Ok, I understand. But since I am SmallLintying my packages I guess that my feedback does not hurt for future work (when you do start changing rules).
Indeed.
Some more feedback:
6) RBAbstractClassRule is confusing. "Checks for references to classes that have subclassResponsibility methods. Such references might be creating instances of the abstract class or more commonly being used as the argument to an isKindOf: message which is considered bad style." In my case, it found FLBenchmarkRunner because it has INSTANCE side messages with subclassResponsability. However, all references to the class FLBenchmarkRunner send class side messages to it. So.... maybe It can exclude class side messages that do not end up sending #new or #basicNew?
7) how does the Manifiesto class envolves while the real packages are changed (class and methods are renamed or removed) ? If a method class is removed then I guess it is easy to remove the metadada associated to them. But what about if a method change? you cannot be sure if the rule exclusion still make sense or not. So, I guess you should remove the metadata as well ?
8) RBMethodHasNoTimeStampRule seems to have a problem with Traits because it shows me lots of methods without timestamp and they all result to come from a trait.
9) Sometimes, clicking on a method raises an error. Find attached PhaorDebug.log
10) RBSubclassResponsibilityNotDefinedRule could show me WHICH are the leaf subclasses that do NOT implement the method ... otherwise it is complicated to find it.
11) I don't like storing the Manifiest class in the default/main package. I changed ManifiestFuel to be in Fuel-SmallLint rather than Fuel but then, as soon as I run the rules again, it moves back the class to Fuel. Can't it check if the class already exist in Smalltalk globals and if true just use it and don't move it ?
12) It would be nice to group rules by criteria. I saw you have #group in each rule so I guess it is a matter of time to have the UI aware of that, right?
Cheers
Stef
Hi guys. After our meeting at Lille, I did a pass of SmallLint over Fuel project. I have to say that I enjoyed the tool and that. I love the manifiesto idea. I love to store metada and to be able to reject rules and scenarios to avoid having false positives again in the future. In addition, it looks like I had less false positive than with other tools. However, there were a couple of rules that I don't understand or may suggest something:
1) First, apart form showing the package between parenthesis, I would also show the protocol (category). Because some rules play with that, so it is handy if you can directly see them there, otherwise you need to browse the code.
2) RBContainsRule is not clear for me. " 'Checks for the common code fragment: "(aCollection detect: [:each | ''some condition''] ifNone: [nil]) ~= nil". contains: can simplify this code to "aCollection contains: [:each | ''some condition'']". Not only is the contains: variant shorter, it better signifies what the code is doing.'" That is not very true. #contains answers true or false, while detect:ifNone:[] answers whatever, and in your example it is nil. So, the answer will be different and hence it is not the same. Ok, in your example, you compare after with == nil... but... I don't really see a use case. For example, in my case it detected:
classNamed: className
^ (migrations detect: [:m | m sourceClassName = className ] ifNone: [ ^ self globalClassNamed: className ]) targetClass.
3) RBLawOfDemeterRule: maybe you can exclude string concatenation? (#,) because it detected a lot of messages like this: signalWith: aGlobalName and: aSelector ^ self signal: 'Method ', aGlobalName, '>>#', aSelector, ' not found.'
Also, what about excluding messages like #asXXX. For example, here it detected "self signature asByteArray". Is that completly breaking the law of demeter? mmm I don't think so.
verifySignatureFrom: aDecoder
| streamSignature | streamSignature := ByteArray new: self signature size. aDecoder nextEncodedBytesInto: streamSignature. (self signature asByteArray = streamSignature) ifFalse: [ FLBadSignature signalCurrentSignature: self signature streamSignature: streamSignature ].
4) RBSentNotImplementedRule: I didn't understand why it has selected:
largeIdentityHash
<primitive: 75> self primitiveFailed
both messages are implemented :(
5) RBClassInstVarNotInitializedRule could be smarter and search not only for #initialize but for #initializeXXX. Lots of times, I initialize my objects with data, using a particular nitialize method, such as: initializeWithBehaviors: someBehaviors extensionMethods: someExtensionMethods so the rule says there is no initialize...but I have that one ;)
That's all the feedback so far. It helped me to find some things to improve so thanks!
-- Mariano http://marianopeck.wordpress.com
-- Mariano http://marianopeck.wordpress.com
<PharoDebug.log>
On 21 April 2012 14:00, Mariano Martinez Peck <marianopeck@gmail.com> wrote:
2) RBContainsRule is not clear for me. " 'Checks for the common code fragment: "(aCollection detect: [:each | ''some condition''] ifNone: [nil]) ~= nil". contains: can simplify this code to "aCollection contains: [:each | ''some condition'']". Not only is the contains: variant shorter, it better signifies what the code is doing.'" That is not very true. #contains answers true or false, while detect:ifNone:[] answers whatever, and in your example it is nil. So, the answer will be different and hence it is not the same. Ok, in your example, you compare after with == nil... but... I don't really see a use case. For example, in my case it detected:
classNamed: className
   ^ (migrations       detect: [:m | m sourceClassName = className ]       ifNone: [ ^ self globalClassNamed: className ])       targetClass.
Thank you for pointing out. I have previously seen similar issues with this rule. I committed an improved version that should return less false-positives, and that finds a few more related issues.
3) RBLawOfDemeterRule: maybe you can exclude string concatenation? (#,) because it detected a lot of messages like this: signalWith: aGlobalName and: aSelector    ^ self signal: 'Method ', aGlobalName, '>>#', aSelector, ' not found.'
Also, what about excluding messages like #asXXX. For example, here it detected "self signature asByteArray". Is that completly breaking the law of demeter? mmm I don't think so.
verifySignatureFrom: aDecoder
   | streamSignature |    streamSignature := ByteArray new: self signature size.    aDecoder nextEncodedBytesInto: streamSignature.    (self signature asByteArray = streamSignature) ifFalse: [       FLBadSignature          signalCurrentSignature: self signature          streamSignature: streamSignature ].
Arguably this is a problem in your code: You could for example use streams or a formatter to build complicated strings; and you could ask the signature if it is the same as a stream signature. It is kind of arbitrary from when on a chain of selectors is considered a violation of the law of demeter ... Maybe the number should be higher? Maybe this number should be even different if these are unary, binary or keyword messages? As with any violation this is just a suggestion.
4) RBSentNotImplementedRule:Â I didn't understand why it has selected:
largeIdentityHash
   <primitive: 75>    self primitiveFailed
both messages are implemented :(
The rule checks if self-sends are implemented in the class hierarchy, and if super-sends are implemented in the superclass hierarchy of the method. For example, when you inherit from ProtoObject then #primitiveFailed is not implemented. In your case the problem might never show up in practice -- for example because your class is abstract and all subclasses implement the message; or because you use #doesNotUnderstand: -- but still I think the rule is correct.
5) RBClassInstVarNotInitializedRule could be smarter and search not only for #initialize but for #initializeXXX. Lots of times, I initialize my objects with data, using a particular nitialize method, such as: initializeWithBehaviors: someBehaviors extensionMethods: someExtensionMethods so the rule says there is no initialize...but I have that one ;)
The rule only checks for the presence of an #initialize method (the only method that is automatically called by the system upon loading of a class) if the class defines variables. It does not check if (and where) the variable is actually assigned. Now if you have classes initialize each other or if you initialize your variables lazily that rule complains. I agree with the author of the rule that this is bad style and worth to think about.
That's all the feedback so far. It helped me to find some things to improve so thanks!
Please don't call it "SmallLint". Since at least 2003 the tool is called "Smalltalk Code Critics", see for example http://www.cincomsmalltalk.com/blog/blogView?entry=3236371324. I think it has been renamed even before 2000, but I can't find the slides of John anymore. Lukas -- Lukas Renggli www.lukas-renggli.ch
Thanks for the feedback On Apr 21, 2012, at 2:00 PM, Mariano Martinez Peck wrote:
Hi guys. After our meeting at Lille, I did a pass of SmallLint over Fuel project. I have to say that I enjoyed the tool and that. I love the manifiesto idea. I love to store metada and to be able to reject rules and scenarios to avoid having false positives again in the future. In addition, it looks like I had less false positive than with other tools. However, there were a couple of rules that I don't understand or may suggest something:
1) First, apart form showing the package between parenthesis, I would also show the protocol (category). Because some rules play with that, so it is handy if you can directly see them there, otherwise you need to browse the code.
2) RBContainsRule is not clear for me. " 'Checks for the common code fragment: "(aCollection detect: [:each | ''some condition''] ifNone: [nil]) ~= nil". contains: can simplify this code to "aCollection contains: [:each | ''some condition'']". Not only is the contains: variant shorter, it better signifies what the code is doing.'" That is not very true. #contains answers true or false, while detect:ifNone:[] answers whatever, and in your example it is nil. So, the answer will be different and hence it is not the same. Ok, in your example, you compare after with == nil... but... I don't really see a use case. For example, in my case it detected:
classNamed: className
^ (migrations detect: [:m | m sourceClassName = className ] ifNone: [ ^ self globalClassNamed: className ]) targetClass.
3) RBLawOfDemeterRule: maybe you can exclude string concatenation? (#,) because it detected a lot of messages like this: signalWith: aGlobalName and: aSelector ^ self signal: 'Method ', aGlobalName, '>>#', aSelector, ' not found.'
Also, what about excluding messages like #asXXX. For example, here it detected "self signature asByteArray". Is that completly breaking the law of demeter? mmm I don't think so.
verifySignatureFrom: aDecoder
| streamSignature | streamSignature := ByteArray new: self signature size. aDecoder nextEncodedBytesInto: streamSignature. (self signature asByteArray = streamSignature) ifFalse: [ FLBadSignature signalCurrentSignature: self signature streamSignature: streamSignature ].
4) RBSentNotImplementedRule: I didn't understand why it has selected:
largeIdentityHash
<primitive: 75> self primitiveFailed
both messages are implemented :(
5) RBClassInstVarNotInitializedRule could be smarter and search not only for #initialize but for #initializeXXX. Lots of times, I initialize my objects with data, using a particular nitialize method, such as: initializeWithBehaviors: someBehaviors extensionMethods: someExtensionMethods so the rule says there is no initialize...but I have that one ;)
That's all the feedback so far. It helped me to find some things to improve so thanks!
-- Mariano http://marianopeck.wordpress.com
participants (4)
-
Lukas Renggli -
Mariano Martinez Peck -
Simon Allier -
Stéphane Ducasse