On 21 April 2012 14:00, Mariano Martinez Peck <[email protected]> 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
