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