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