The Trunk: Collections-eem.756.mcz

On Wed, 14 Jun 2017, [hidden email] wrote:

Eliot Miranda uploaded a new version of Collections to project The Trunk:
http://source.squeak.org/trunk/Collections-eem.756.mcz

==================== Summary ====================

Name: Collections-eem.756
Author: eem
Time: 14 June 2017, 11:03:24.917631 am
UUID: 8d7c03bc-1cdb-44c7-9173-10d50c0dae29
Ancestors: Collections-eem.755

Add SequenceableCollection>>withoutDuplicates for a more elegant fix to MailMessage>>to:

=============== Diff against Collections-eem.755 ===============

Item was added:
+ ----- Method: SequenceableCollection>>withoutDuplicates (in category 'copying') -----
+ withoutDuplicates
+       "Answer a copy of the receiver that preserves order but eliminates any duplicates."
+       | seen |
+       seen := Set new: self size.
+       ^self select: [:each|
+                                 (seen includes: each)
+                                       ifTrue: [false]
+                                       ifFalse: [seen add: each. true]]!

What a great opportunity to use #addNewElement::

        ^self select: [ :each | seen addNewElement: each ]

Levente

Hi Levente, Hi Chris,

On Wed, Jun 14, 2017 at 1:10 PM, Levente Uzonyi <[hidden email]> wrote:

On Wed, 14 Jun 2017, [hidden email] wrote:

Eliot Miranda uploaded a new version of Collections to project The Trunk:
http://source.squeak.org/trunk/Collections-eem.756.mcz

==================== Summary ====================

Name: Collections-eem.756
Author: eem
Time: 14 June 2017, 11:03:24.917631 am
UUID: 8d7c03bc-1cdb-44c7-9173-10d50c0dae29
Ancestors: Collections-eem.755

Add SequenceableCollection>>withoutDuplicates for a more elegant fix to MailMessage>>to:

=============== Diff against Collections-eem.755 ===============

Item was added:
+ ----- Method: SequenceableCollection>>withoutDuplicates (in category 'copying') -----
+ withoutDuplicates
+       "Answer a copy of the receiver that preserves order but eliminates any duplicates."
+       | seen |
+       seen := Set new: self size.
+       ^self select: [:each|
+                                 (seen includes: each)
+                                       ifTrue: [false]
+                                       ifFalse: [seen add: each. true]]!

What a great opportunity to use #addNewElement::

        ^self select: [ :each | seen addNewElement: each ]

I love the functionality but I don't like the selector. It seems to imply that one must only add a new element.  So why not call this something like addIfAbsent: ?

Here are some suggestions.  Votes?

- don't change it; stick with addNewElement:

- addIfAbsent:

- ifAbsentAdd:

- ifMissingAdd:

I think I prefer ifAbsentAdd: cuz addIfAbsent: looks too much like a potential spelling error, and conflicts with typical ifAbsent: arguments supplying exception blocks.  But I could go with ifMissingAdd: because it is more distinctive.

 

Levente

_,,,^..^,,,_

best, Eliot

Hi Eliot,

Yes, because of the dual purpose of this method; 1) to ensure presence of an element and, 2) answer whether the element already existed,

there isn't a clear and concise name which conveys the full nuance.  It's one of those methods which requires an initial look at the comment and/or code until one is familiar with it.

I considered those names, too.  I didn't care for addIfAbsent: for the reason you stated.  I don't care for ifAbsentAdd: because it's backward with other "Absent" nomenclature in the API.

And, I didn't go with #ifMissingAdd: because it confuses the return value with the linguistic semantics -- e.g., if it returns true, does that mean it was "absent" or that it was added?  forcing me to look at the method implementation again anyway.

#addNewElement: fits in with the #add... nomenclature part of the API, being like a "command" to the object, to which it may answer, "no" (false), if the element already existed.

It seems the only initial confusion you had was whether it signaled an Exception or not, but I see no other name that avoids the user needing to look at the implementation at least the first time.

Therefore, I think #addNewElement: is a good name.

Best,

  Chris

On Thu, Jun 15, 2017 at 1:30 PM, Eliot Miranda <[hidden email]> wrote:

Hi Levente, Hi Chris,

On Wed, Jun 14, 2017 at 1:10 PM, Levente Uzonyi <[hidden email]> wrote:

On Wed, 14 Jun 2017, [hidden email] wrote:

Eliot Miranda uploaded a new version of Collections to project The Trunk:
http://source.squeak.org/trunk/Collections-eem.756.mcz

==================== Summary ====================

Name: Collections-eem.756
Author: eem
Time: 14 June 2017, 11:03:24.917631 am
UUID: 8d7c03bc-1cdb-44c7-9173-10d50c0dae29
Ancestors: Collections-eem.755

Add SequenceableCollection>>withoutDuplicates for a more elegant fix to MailMessage>>to:

=============== Diff against Collections-eem.755 ===============

Item was added:
+ ----- Method: SequenceableCollection>>withoutDuplicates (in category 'copying') -----
+ withoutDuplicates
+       "Answer a copy of the receiver that preserves order but eliminates any duplicates."
+       | seen |
+       seen := Set new: self size.
+       ^self select: [:each|
+                                 (seen includes: each)
+                                       ifTrue: [false]
+                                       ifFalse: [seen add: each. true]]!

What a great opportunity to use #addNewElement::

        ^self select: [ :each | seen addNewElement: each ]

I love the functionality but I don't like the selector. It seems to imply that one must only add a new element.  So why not call this something like addIfAbsent: ?

Here are some suggestions.  Votes?

- don't change it; stick with addNewElement:

- addIfAbsent:

- ifAbsentAdd:

- ifMissingAdd:

I think I prefer ifAbsentAdd: cuz addIfAbsent: looks too much like a potential spelling error, and conflicts with typical ifAbsent: arguments supplying exception blocks.  But I could go with ifMissingAdd: because it is more distinctive.

 

Levente

_,,,^..^,,,_

best, Eliot

> On 15.06.2017, at 20:30, Eliot Miranda <[hidden email]> wrote:
>
> Hi Levente, Hi Chris,
>
> On Wed, Jun 14, 2017 at 1:10 PM, Levente Uzonyi <[hidden email]> wrote:
> On Wed, 14 Jun 2017, [hidden email] wrote:
>
> Eliot Miranda uploaded a new version of Collections to project The Trunk:
> http://source.squeak.org/trunk/Collections-eem.756.mcz
>
> ==================== Summary ====================
>
> Name: Collections-eem.756
> Author: eem
> Time: 14 June 2017, 11:03:24.917631 am
> UUID: 8d7c03bc-1cdb-44c7-9173-10d50c0dae29
> Ancestors: Collections-eem.755
>
> Add SequenceableCollection>>withoutDuplicates for a more elegant fix to MailMessage>>to:
>
> =============== Diff against Collections-eem.755 ===============
>
> Item was added:
> + ----- Method: SequenceableCollection>>withoutDuplicates (in category 'copying') -----
> + withoutDuplicates
> +       "Answer a copy of the receiver that preserves order but eliminates any duplicates."
> +       | seen |
> +       seen := Set new: self size.
> +       ^self select: [:each|
> +                                 (seen includes: each)
> +                                       ifTrue: [false]
> +                                       ifFalse: [seen add: each. true]]!
>
> What a great opportunity to use #addNewElement::
>
>         ^self select: [ :each | seen addNewElement: each ]
>
> I love the functionality but I don't like the selector. It seems to imply that one must only add a new element.  So why not call this something like addIfAbsent: ?
>
> Here are some suggestions.  Votes?
> - don't change it; stick with addNewElement:
> - addIfAbsent:
> - ifAbsentAdd:
> - ifMissingAdd:
>
> I think I prefer ifAbsentAdd: cuz addIfAbsent: looks too much like a potential spelling error, and conflicts with typical ifAbsent: arguments supplying exception blocks.  But I could go with ifMissingAdd: because it is more distinctive.

Well, we do have

        Collection>>addIfNotPresent:

So why invent a new one?

I think the important thing with #addNewElement: is that it returns *whether* it added a new element
but then again it breaks the tradition of #add* returning its argument…

a very clear and very strange one that would reveal that a boolean is returned would be #isAbsentAndIfSoAdd: …

Best regards
        -Tobias

> _,,,^..^,,,_
> best, Eliot

> On 15.06.2017, at 21:22, Chris Cunningham <[hidden email]> wrote:
>
> #testMissingThenAdd:  ?
>

#isAdding:
#ingests:

#wasAbsentButNowIsPresent:

(just rambling…)

> On Jun 15, 2017 12:19 PM, "Eliot Miranda" <[hidden email]> wrote:
> Hi Tobias,
>
> On Thu, Jun 15, 2017 at 12:06 PM, Tobias Pape <[hidden email]> wrote:
>
> > On 15.06.2017, at 20:30, Eliot Miranda <[hidden email]> wrote:
> >
> > Hi Levente, Hi Chris,
> >
> > On Wed, Jun 14, 2017 at 1:10 PM, Levente Uzonyi <[hidden email]> wrote:
> > On Wed, 14 Jun 2017, [hidden email] wrote:
> >
> > Eliot Miranda uploaded a new version of Collections to project The Trunk:
> > http://source.squeak.org/trunk/Collections-eem.756.mcz
> >
> > ==================== Summary ====================
> >
> > Name: Collections-eem.756
> > Author: eem
> > Time: 14 June 2017, 11:03:24.917631 am
> > UUID: 8d7c03bc-1cdb-44c7-9173-10d50c0dae29
> > Ancestors: Collections-eem.755
> >
> > Add SequenceableCollection>>withoutDuplicates for a more elegant fix to MailMessage>>to:
> >
> > =============== Diff against Collections-eem.755 ===============
> >
> > Item was added:
> > + ----- Method: SequenceableCollection>>withoutDuplicates (in category 'copying') -----
> > + withoutDuplicates
> > +       "Answer a copy of the receiver that preserves order but eliminates any duplicates."
> > +       | seen |
> > +       seen := Set new: self size.
> > +       ^self select: [:each|
> > +                                 (seen includes: each)
> > +                                       ifTrue: [false]
> > +                                       ifFalse: [seen add: each. true]]!
> >
> > What a great opportunity to use #addNewElement::
> >
> >         ^self select: [ :each | seen addNewElement: each ]
> >
> > I love the functionality but I don't like the selector. It seems to imply that one must only add a new element.  So why not call this something like addIfAbsent: ?
> >
> > Here are some suggestions.  Votes?
> > - don't change it; stick with addNewElement:
> > - addIfAbsent:
> > - ifAbsentAdd:
> > - ifMissingAdd:
> >
> > I think I prefer ifAbsentAdd: cuz addIfAbsent: looks too much like a potential spelling error, and conflicts with typical ifAbsent: arguments supplying exception blocks.  But I could go with ifMissingAdd: because it is more distinctive.
>
> Well, we do have
>
>         Collection>>addIfNotPresent:
>
> So why invent a new one?
>
> Because addIfNotPresent: answers its argument and we need one that answers whether the element was absent.  So alas addIfNotPresent: is not a suitable candidate.
>
>
> I think the important thing with #addNewElement: is that it returns *whether* it added a new element
> but then again it breaks the tradition of #add* returning its argument…
>
> a very clear and very strange one that would reveal that a boolean is returned would be #isAbsentAndIfSoAdd: …
>
> wasAbsentAdding: or ifWasAbsentAdding: would be less cumbersome but I like something snappier that people will remember.  ifMissingAdd: looks good because it doesn't conflict with the add*: methods answering their argument, and the ifMissing implies the answer is true if the element wasn't already present.
>
> Stéphane, can you live with ifMissingAdd: ?  Chris?
>
> Best regards
>         -Tobias
>
> > _,,,^..^,,,_
> > best, Eliot
>
> _,,,^..^,,,_
> best, Eliot

Yes, because of the dual purpose of this method; 1) to ensure presence of an element and, 2) answer whether the element already existed,

to be pedantic, answer if the element didn't already exist

there isn't a clear and concise name which conveys the full nuance.  It's one of those methods which requires an initial look at the comment and/or code until one is familiar with it.

Given that addNewElement: doesn't convey the full nuance that's not a valid criticism of the other alternatives, is it ;-)

I considered those names, too.  I didn't care for addIfAbsent: for the reason you stated.  I don't care for ifAbsentAdd: because it's backward with other "Absent" nomenclature in the API.

And, I didn't go with #ifMissingAdd: because it confuses the return value with the linguistic semantics -- e.g., if it returns true, does that mean it was "absent" or that it was added?  forcing me to look at the method implementation again anyway.

#addNewElement: fits in with the #add... nomenclature part of the API, being like a "command" to the object, to which it may answer, "no" (false), if the element already existed.

It seems the only initial confusion you had was whether it signaled an Exception or not, but I see no other name that avoids the user needing to look at the implementation at least the first time.

No.  The selector implies it might. The selector is bad because this does not add a new element, it ensures an element is present,

and answers if it was added.

Therefore, I think #addNewElement: is a good name.

I beg to differ.  I find it misleading :-(  Which is a shame because it's really useful.  How about trying to reach consensus on a less misleading name?

> On 15.06.2017, at 21:40, Eliot Miranda <[hidden email]> wrote:
>
> Hi Tobias,
>
> On Thu, Jun 15, 2017 at 12:32 PM, Tobias Pape <[hidden email]> wrote:
>
> > On 15.06.2017, at 21:22, Chris Cunningham <[hidden email]> wrote:
> >
> > #testMissingThenAdd:  ?
> >
>
> #isAdding:
> #ingests:
>
> #wasAbsentButNowIsPresent:
>
> (just rambling…)
>
> I like added:.  Its concise, accurate and reads nicely:

Ok, but then maybe #addedNew: ? So as we know more than just an adding operation succeeded… :)

>
> withoutDuplicates
>       "Answer a copy of the receiver that preserves order but eliminates any duplicates."
>       | seen |
>       seen := Set new: self size.
>       ^self select: [:each| seen added: each]
>
> messagesDo: aBlock
>       "Evaluate aBlock exactly once with all the message selectors sent by me."
>
>       | scanner aSet |
>       self isQuick ifTrue: [ ^self ].
>       scanner := InstructionStream on: self.
>       scanner scanFor: [ :x |
>               | selector |
>               (selector := scanner selectorToSendOrSelf) == scanner ifFalse: [
>                       ((aSet ifNil: [ aSet := IdentitySet new ]) added: selector) ifTrue: [
>                               aBlock value: selector ] ].
>               false "keep scanning" ]
>
>
>
> > On Jun 15, 2017 12:19 PM, "Eliot Miranda" <[hidden email]> wrote:
> > Hi Tobias,
> >
> > On Thu, Jun 15, 2017 at 12:06 PM, Tobias Pape <[hidden email]> wrote:
> >
> > > On 15.06.2017, at 20:30, Eliot Miranda <[hidden email]> wrote:
> > >
> > > Hi Levente, Hi Chris,
> > >
> > > On Wed, Jun 14, 2017 at 1:10 PM, Levente Uzonyi <[hidden email]> wrote:
> > > On Wed, 14 Jun 2017, [hidden email] wrote:
> > >
> > > Eliot Miranda uploaded a new version of Collections to project The Trunk:
> > > http://source.squeak.org/trunk/Collections-eem.756.mcz
> > >
> > > ==================== Summary ====================
> > >
> > > Name: Collections-eem.756
> > > Author: eem
> > > Time: 14 June 2017, 11:03:24.917631 am
> > > UUID: 8d7c03bc-1cdb-44c7-9173-10d50c0dae29
> > > Ancestors: Collections-eem.755
> > >
> > > Add SequenceableCollection>>withoutDuplicates for a more elegant fix to MailMessage>>to:
> > >
> > > =============== Diff against Collections-eem.755 ===============
> > >
> > > Item was added:
> > > + ----- Method: SequenceableCollection>>withoutDuplicates (in category 'copying') -----
> > > + withoutDuplicates
> > > +       "Answer a copy of the receiver that preserves order but eliminates any duplicates."
> > > +       | seen |
> > > +       seen := Set new: self size.
> > > +       ^self select: [:each|
> > > +                                 (seen includes: each)
> > > +                                       ifTrue: [false]
> > > +                                       ifFalse: [seen add: each. true]]!
> > >
> > > What a great opportunity to use #addNewElement::
> > >
> > >         ^self select: [ :each | seen addNewElement: each ]
> > >
> > > I love the functionality but I don't like the selector. It seems to imply that one must only add a new element.  So why not call this something like addIfAbsent: ?
> > >
> > > Here are some suggestions.  Votes?
> > > - don't change it; stick with addNewElement:
> > > - addIfAbsent:
> > > - ifAbsentAdd:
> > > - ifMissingAdd:
> > >
> > > I think I prefer ifAbsentAdd: cuz addIfAbsent: looks too much like a potential spelling error, and conflicts with typical ifAbsent: arguments supplying exception blocks.  But I could go with ifMissingAdd: because it is more distinctive.
> >
> > Well, we do have
> >
> >         Collection>>addIfNotPresent:
> >
> > So why invent a new one?
> >
> > Because addIfNotPresent: answers its argument and we need one that answers whether the element was absent.  So alas addIfNotPresent: is not a suitable candidate.
> >
> >
> > I think the important thing with #addNewElement: is that it returns *whether* it added a new element
> > but then again it breaks the tradition of #add* returning its argument…
> >
> > a very clear and very strange one that would reveal that a boolean is returned would be #isAbsentAndIfSoAdd: …
> >
> > wasAbsentAdding: or ifWasAbsentAdding: would be less cumbersome but I like something snappier that people will remember.  ifMissingAdd: looks good because it doesn't conflict with the add*: methods answering their argument, and the ifMissing implies the answer is true if the element wasn't already present.
> >
> > Stéphane, can you live with ifMissingAdd: ?  Chris?
> >
> > Best regards
> >         -Tobias
> >
> > > _,,,^..^,,,_
> > > best, Eliot
> >
> > _,,,^..^,,,_
> > best, Eliot
>
> _,,,^..^,,,_
> best, Eliot
>

maybe addOnce: would be more expressive than addNew:?

2017-06-15 22:52 GMT+02:00 Chris Muller <[hidden email]>:

^ self select: [ : each | seen addNew: each ]

?

On Thu, Jun 15, 2017 at 3:50 PM, Chris Cunningham
<[hidden email]> wrote:
>
>
> On Thu, Jun 15, 2017 at 1:22 PM, Tobias Pape <[hidden email]> wrote:
>>
>>
>> > On 15.06.2017, at 21:40, Eliot Miranda <[hidden email]> wrote:
>> >
>> > Hi Tobias,
>> >
>> > On Thu, Jun 15, 2017 at 12:32 PM, Tobias Pape <[hidden email]> wrote:
>> >
>> > > On 15.06.2017, at 21:22, Chris Cunningham <[hidden email]>
>> > > wrote:
>> > >
>> > > #testMissingThenAdd:  ?
>> > >
>> >
>> > #isAdding:
>> > #ingests:
>> >
>> > #wasAbsentButNowIsPresent:
>> >
>> > (just rambling…)
>> >
>> > I like added:.  Its concise, accurate and reads nicely:
>>
>>
>> Ok, but then maybe #addedNew: ? So as we know more than just an adding
>> operation succeeded… :)
>
>
> So it reads:
>
> ^self select: [:each| seen addedNew: each]
>
> I like that how that reads.
>
> #added: wasn't bad - but I could see myself thinking it meant roughly 'was
> this value already in the collection'?  Which misses the add part and is
> backwards on the result (true if present, false if added).
>
> -cbc
>>
>>
>> >
>> > withoutDuplicates
>> >       "Answer a copy of the receiver that preserves order but eliminates
>> > any duplicates."
>> >       | seen |
>> >       seen := Set new: self size.
>> >       ^self select: [:each| seen added: each]
>> >
>> > messagesDo: aBlock
>> >       "Evaluate aBlock exactly once with all the message selectors sent
>> > by me."
>> >
>> >       | scanner aSet |
>> >       self isQuick ifTrue: [ ^self ].
>> >       scanner := InstructionStream on: self.
>> >       scanner scanFor: [ :x |
>> >               | selector |
>> >               (selector := scanner selectorToSendOrSelf) == scanner
>> > ifFalse: [
>> >                       ((aSet ifNil: [ aSet := IdentitySet new ]) added:
>> > selector) ifTrue: [
>> >                               aBlock value: selector ] ].
>> >               false "keep scanning" ]
>> >
>> >
>> >
>> > > On Jun 15, 2017 12:19 PM, "Eliot Miranda" <[hidden email]>
>> > > wrote:
>> > > Hi Tobias,
>> > >
>> > > On Thu, Jun 15, 2017 at 12:06 PM, Tobias Pape <[hidden email]>
>> > > wrote:
>> > >
>> > > > On 15.06.2017, at 20:30, Eliot Miranda <[hidden email]>
>> > > > wrote:
>> > > >
>> > > > Hi Levente, Hi Chris,
>> > > >
>> > > > On Wed, Jun 14, 2017 at 1:10 PM, Levente Uzonyi
>> > > > <[hidden email]> wrote:
>> > > > On Wed, 14 Jun 2017, [hidden email] wrote:
>> > > >
>> > > > Eliot Miranda uploaded a new version of Collections to project The
>> > > > Trunk:
>> > > > http://source.squeak.org/trunk/Collections-eem.756.mcz
>> > > >
>> > > > ==================== Summary ====================
>> > > >
>> > > > Name: Collections-eem.756
>> > > > Author: eem
>> > > > Time: 14 June 2017, 11:03:24.917631 am
>> > > > UUID: 8d7c03bc-1cdb-44c7-9173-10d50c0dae29
>> > > > Ancestors: Collections-eem.755
>> > > >
>> > > > Add SequenceableCollection>>withoutDuplicates for a more elegant fix
>> > > > to MailMessage>>to:
>> > > >
>> > > > =============== Diff against Collections-eem.755 ===============
>> > > >
>> > > > Item was added:
>> > > > + ----- Method: SequenceableCollection>>withoutDuplicates (in
>> > > > category 'copying') -----
>> > > > + withoutDuplicates
>> > > > +       "Answer a copy of the receiver that preserves order but
>> > > > eliminates any duplicates."
>> > > > +       | seen |
>> > > > +       seen := Set new: self size.
>> > > > +       ^self select: [:each|
>> > > > +                                 (seen includes: each)
>> > > > +                                       ifTrue: [false]
>> > > > +                                       ifFalse: [seen add: each.
>> > > > true]]!
>> > > >
>> > > > What a great opportunity to use #addNewElement::
>> > > >
>> > > >         ^self select: [ :each | seen addNewElement: each ]
>> > > >
>> > > > I love the functionality but I don't like the selector. It seems to
>> > > > imply that one must only add a new element.  So why not call this something
>> > > > like addIfAbsent: ?
>> > > >
>> > > > Here are some suggestions.  Votes?
>> > > > - don't change it; stick with addNewElement:
>> > > > - addIfAbsent:
>> > > > - ifAbsentAdd:
>> > > > - ifMissingAdd:
>> > > >
>> > > > I think I prefer ifAbsentAdd: cuz addIfAbsent: looks too much like a
>> > > > potential spelling error, and conflicts with typical ifAbsent: arguments
>> > > > supplying exception blocks.  But I could go with ifMissingAdd: because it is
>> > > > more distinctive.
>> > >
>> > > Well, we do have
>> > >
>> > >         Collection>>addIfNotPresent:
>> > >
>> > > So why invent a new one?
>> > >
>> > > Because addIfNotPresent: answers its argument and we need one that
>> > > answers whether the element was absent.  So alas addIfNotPresent: is not a
>> > > suitable candidate.
>> > >
>> > >
>> > > I think the important thing with #addNewElement: is that it returns
>> > > *whether* it added a new element
>> > > but then again it breaks the tradition of #add* returning its
>> > > argument…
>> > >
>> > > a very clear and very strange one that would reveal that a boolean is
>> > > returned would be #isAbsentAndIfSoAdd: …
>> > >
>> > > wasAbsentAdding: or ifWasAbsentAdding: would be less cumbersome but I
>> > > like something snappier that people will remember.  ifMissingAdd: looks good
>> > > because it doesn't conflict with the add*: methods answering their argument,
>> > > and the ifMissing implies the answer is true if the element wasn't already
>> > > present.
>> > >
>> > > Stéphane, can you live with ifMissingAdd: ?  Chris?
>> > >
>> > > Best regards
>> > >         -Tobias
>> > >
>> > > > _,,,^..^,,,_
>> > > > best, Eliot
>> > >
>> > > _,,,^..^,,,_
>> > > best, Eliot
>> >
>> > _,,,^..^,,,_
>> > best, Eliot
>> >
>>
>>
>
>
>
>