Hi Daniel,

though I think, it's quite new, as I've missed it before, but:

Collection >> allSatisfy: aBlock

seems to be there now.

Using accessors for referencing the variables, you could also write

RenameVariableChange>>= aRenameVariableChange ^#( #(#class #class) #(#className #changeClassName) #(#isMeta #isMeta) #(#oldName #oldName) #(#newName #newName)) allSatisfy: [:someSymbols| (self perform: someSymbols first) = (aRenameVariableChange perform: someSymbols second)]

So why do I need this stupid duplication of symbols? Because of a strange behaviour of

RenameVariableChange >> changeClassName ^className

I think the RB-makers intended to make clear, that this is not the name of the class of RenameVariableChange, but they shouldn't have bothered about this; "change" is already defined in the name of the class, so it is a light unneccesary duplication. If we renamed this method to

RenameVariableChange >> className ^className

the code above becomes:

RenameVariableChange>>= aRenameVariableChange ^#(#class #className #isMeta #oldName #newName ) allSatisfy: [:aSymbol| (self perform: aSymbol)= (aRenameVariableChange perform: aSymbol)]

Better.

But I saw this before, so why not extracting it:

Object >> isEqualTo: anObject inAspects: someSymbols ^someAspects allSatisfy: [:aSymbol | (self perform: aSymbol) = (anObject perform: aSymbol)]

And we would get:

RenameVariableChange>>= aRenameVariableChange ^self isEqualTo: anObject inAspects: #(#class #className #isMeta #oldName #newName )

Best,

Markus

Daniel, Try reformating the method in question to make it easier to read. Your proposal just abstracts the nested "and" expression which only adds another layer to hide its intention. the #add: expressions will quit the evaluation sequence at the first expression that fails. No neat to complicate things further with allTrue. Neatly formatted code will make make the intention of the code clear. Phil

danielv@netvision.net.il wrote:

I was looking at a Lint report, and it found this method -

RenameVariableChange>>= = aRenameVariableChange self class = aRenameVariableChange class ifFalse: [^false]. ^className = aRenameVariableChange changeClassName and: [isMeta = aRenameVariableChange isMeta and: [oldName = aRenameVariableChange oldName and: [newName = aRenameVariableChange newName]]]

Lint was bothered by the use of a guard clause for the first check. Lint believes in #and:.

I was bothered because it's ugly. I'd prefer something less cluttered. We have blocks, we can abstract it!

How about this -

^{[self class = aRenameVariableChange class]. [className = aRenameVariableChange changeClassName]. [isMeta = aRenameVariableChange isMeta]. [oldName = aRenameVariableChange oldName]. [newName = aRenameVariableChange newName]} allTrue

Note that the conditions should be tested in order and lazily - stops at the first false.

The implementation is as follows - Collection>>allTrue ^self detect: [:e | e value not] ifNone: [nil]) isNil

What do you think? Any other ideas on how to factor this sort of code?

Daniel