[squeak-dev] #doWhileTrue: (was: The Trunk: System-nice.1149.mcz)

Nicolas Cellier nicolas.cellier.aka.nice at gmail.com
Mon Mar 30 18:56:43 UTC 2020


Le lun. 30 mars 2020 à 19:58, Thiede, Christoph <
Christoph.Thiede at student.hpi.uni-potsdam.de> a écrit :

> Hi Nicolas,
>
>
> > There is no other sender of doWhileFalse: doWhileTrue: and my advice
> would be to deprecate them. YAGNI.
>
> So my image actually contains around two dozen senders of #doWhileTrue:
> and #doWhileFalse: (Squot, Pheno and some of my own code). As mentioned
> somewhere else in the past, I actually like them and find them very useful.
>
> In my opinion, many senders of #whileTrue and #whileFalse should be
> refactored to use #doWhileTrue:/#doWhileFalse:. Here is a random example:
>
> [newPlace := self getNewPlace.
> dir := ServerFile new fullPath: newPlace.
> (dir includesKey: dir fileName)] whileTrue.
>
> I find that's ugly and counter-intuitive! I would rewrite this in the
> following way:
>
> [newPlace := self getNewPlace.
> dir := ServerFile new fullPath: newPlace]
>     doWhileTrue: [dir includesKey: dir fileName].
>
>
The first thing that upsets me is that they are not idiomatic. #whileTrue:
and in a lesser way #whileTrue are.
The second thing that upsets me is that the names are REALLY too close to
#whileTrue:
So at first, I was confused by the fact that they looked like synonyms.
Then I wondered exactly the same, what the hell has to be true?
Especially when the sole sender in trunk is [] doWhileTrue: true ;)

IMO, they might be better spelled #repeatWhile: and #repeatUntil:
But that still means that we have two ways to express exactly the same
thing, which by first principle should not be
(economy of implementation, of tests, of documentation, etc... small is
beautiful !).

We might discuss which is more readable, maybe

[newPlace := self getNewPlace.
dir := ServerFile new fullPath: newPlace]
    repeatWhile: [dir includesKey: dir fileName].

is more revealing than:

[newPlace := self getNewPlace.
dir := ServerFile new fullPath: newPlace.
(dir includesKey: dir fileName)] whileTrue.

IMO, it's a biased example, because the whole snippet is un-comprehensible.
So a ServerFile behaves like a ServerDirectory (from which it inherits...),
oh nice (???).
And a ServerFile which would includes some item with same name as itself
would mean that the file already exists...
Really, not kidding?

So IMO that's what makes the snippet illegible, not whileTrue.
Is the following better?

[newPlace := self getNewPlace.
file:= ServerFile new fullPath: newPlace.
file exists] whileTrue

Anyway, the second one is there for such a long time that you can't expunge
it that easily. It's idiomatic...

> The latter just reads easier. blockReturns in multi-statement blocks are
> not really intuitive.
>

Note that I did not add non local block return in the middle of the block...
It was in the original method, doWhileTrue did not solve it.

In the first version, when scanning the method quickly, I read "okay,
> something with places and directories is done, while true ...," I wonder
> "what has to be true?" and have to look back to the end of the block and
> find the beginning of the last statement.
> But in the second version, I read "okay, something with places and
> directories is done, while that directory matches this specific condition."
> This version directly draws my attention to the important condition.
>
> Happy to hear your counterarguments :-)
>
> Best,
> Christoph
>
> ------------------------------
> *Von:* Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im
> Auftrag von commits at source.squeak.org <commits at source.squeak.org>
> *Gesendet:* Montag, 30. März 2020 12:51 Uhr
> *An:* squeak-dev at lists.squeakfoundation.org;
> packages at lists.squeakfoundation.org
> *Betreff:* [squeak-dev] The Trunk: System-nice.1149.mcz
>
> Nicolas Cellier uploaded a new version of System to project The Trunk:
> http://source.squeak.org/trunk/System-nice.1149.mcz
>
> ==================== Summary ====================
>
> Name: System-nice.1149
> Author: nice
> Time: 30 March 2020, 12:50:26.196366 pm
> UUID: ece53b0b-24d2-4d4b-bc91-6794a661f886
> Ancestors: System-ul.1148
>
> avoid neuron storming [...] doWhileTrue: true, it just means [...] repeat
>
> There is no other sender of doWhileFalse: doWhileTrue: and my advice would
> be to deprecate them. YAGNI.
>
> =============== Diff against System-ul.1148 ===============
>
> Item was changed:
>   ----- Method: MOFile>>searchByHash: (in category 'experimental') -----
>   searchByHash: aString
>          | hashValue nstr index incr key |
>          hashValue :=  self hashPjw: aString.
>          incr := 1 + (hashValue \\ (hashTableSize -2)).
>          index := (hashValue \\ hashTableSize) .
>          [        nstr := (hashTable at: index +1 ).
>                  nstr = 0 ifTrue: [^nil].
>                  key := self originalString: nstr.
>                  key = aString ifTrue: [^self translatedString: nstr].
>                  index >= (hashTableSize - incr)
>                                  ifTrue: [index := index - (hashTableSize
> - incr)  ]
>                                  ifFalse:[index := index + incr].
> +        ] repeat!
> -        ] doWhileTrue: true.!
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20200330/b62c3fc5/attachment.html>


More information about the Squeak-dev mailing list