I appreciate what you're saying, and in more primitive tools (when doing
C, VB), I've also use formatting carefully. But Squeak doesn't treat
formatting very respectfully - SyntaxMorphs don't know what that is, the
pretty-printers reverse it, and the Refactoring Browser (which I
actually use) doesn't retain it across refactorings (not unreasonable).

So I'm loath to spend much time on such an (here and now) ephemeral way
of expressing my intention.

OTOH, abstracting away reccuring control structures (like the nested
"and" expressions) sounds like the precise sort of jobs that blocks were
meant to do. BTW, I'm quite aware that the and:'s terminate on the first
false. It's an important part of the pattern, and the implementation
proposed mimics it.

I have one problem with the pattern I proposed and that's that it
doesn't make thing simple enough yet. All those blocks are still
clutter. 

Maybe -

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

Communicates the meanings better. I'm only delaying computation where
that is needed. The allTrue could still be compatible with blocks,
evaluating any it finds as it goes, but it should accept values, too,
where that power isn't needed.

So if newName was an expensive computation, I'd put a block around the
last item, to avoid calculating it when not needed.

Daniel

Phil Weichert <weichert at hal-pc.org> wrote:
> 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 at 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