Stylistic question: private helper vs extension method?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Stylistic question: private helper vs extension method?

Herby Vojčík
Hello!

I've got this portion in my delegate:

        requestPayload ifNotNil: [ uuidKeys do: [ :each |
                        requestPayload at: each ifPresent: [ :s | requestPayload at: each
put: (UUID fromString: s) ] ] ].
        responsePayload := self towergame clientSync: requestPayload.
        responsePayload ifNotNil: [ uuidKeys do: [ :each |
                        responsePayload at: each ifPresent: [ :uuid | responsePayload at:
each put: uuid asString ] ] ].

Now I would gladly use something like Dictionary >>
at:ifPresentTransform: aBlock. But it is not present, so I have two choices:

   1. Add it as extension method, but then it may clash if someone else
has similar idea.
   2. Add private helper TowergameDelegate >>
dict:at:ifPresentTransform:, which is longer and needs additional self
receiver.

Which is preferable?

Herby


Reply | Threaded
Open this post in threaded view
|

Re: Stylistic question: private helper vs extension method?

EstebanLM
Extension. Helpers are for javaers.

Esteban

> On 8 Aug 2017, at 22:39, Herby Vojčík <[hidden email]> wrote:
>
> Hello!
>
> I've got this portion in my delegate:
>
>    requestPayload ifNotNil: [ uuidKeys do: [ :each |
>            requestPayload at: each ifPresent: [ :s | requestPayload at: each put: (UUID fromString: s) ] ] ].
>    responsePayload := self towergame clientSync: requestPayload.
>    responsePayload ifNotNil: [ uuidKeys do: [ :each |
>            responsePayload at: each ifPresent: [ :uuid | responsePayload at: each put: uuid asString ] ] ].
>
> Now I would gladly use something like Dictionary >> at:ifPresentTransform: aBlock. But it is not present, so I have two choices:
>
>  1. Add it as extension method, but then it may clash if someone else has similar idea.
>  2. Add private helper TowergameDelegate >> dict:at:ifPresentTransform:, which is longer and needs additional self receiver.
>
> Which is preferable?
>
> Herby
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Stylistic question: private helper vs extension method?

Richard Sargent
Administrator
In reply to this post by Herby Vojčík
Herby Vojčík wrote
Hello!

I've got this portion in my delegate:

        requestPayload ifNotNil: [ uuidKeys do: [ :each |
                        requestPayload at: each ifPresent: [ :s | requestPayload at: each
put: (UUID fromString: s) ] ] ].
        responsePayload := self towergame clientSync: requestPayload.
        responsePayload ifNotNil: [ uuidKeys do: [ :each |
                        responsePayload at: each ifPresent: [ :uuid | responsePayload at:
each put: uuid asString ] ] ].

Now I would gladly use something like Dictionary >>
at:ifPresentTransform: aBlock. But it is not present, so I have two choices:

   1. Add it as extension method, but then it may clash if someone else
has similar idea.
I would make this choice. The extension is directly relevant to dictionary-ness, so an extenstion makes sense. But, I would go with a different name and a more familiar pattern: #transformAt:using: and #transformAt:using:ifAbsent:. You would use the latter, with an empty block for the #ifAbsent: keyword. The former, if you chose to implement it, would throw an element not found exception if the key is not in the dictionary.

I think it helps when the verb begins the behaviour name. Admittedly, there are numerous cases where the symmetry is more important, as with #at: and #at:put:. (But, probably because they are short.)


   2. Add private helper TowergameDelegate >>
dict:at:ifPresentTransform:, which is longer and needs additional self
receiver.

Which is preferable?

Herby
Reply | Threaded
Open this post in threaded view
|

Re: Stylistic question: private helper vs extension method?

Ben Coman
In reply to this post by Herby Vojčík


On Wed, Aug 9, 2017 at 4:39 AM, Herby Vojčík <[hidden email]> wrote:
Hello!

I've got this portion in my delegate:

        requestPayload ifNotNil: [ uuidKeys do: [ :each |
                        requestPayload at: each ifPresent: [ :s | requestPayload at: each put: (UUID fromString: s) ] ] ].
        responsePayload := self towergame clientSync: requestPayload.
        responsePayload ifNotNil: [ uuidKeys do: [ :each |
                        responsePayload at: each ifPresent: [ :uuid | responsePayload at: each put: uuid asString ] ] ].

Now I would gladly use something like Dictionary >> at:ifPresentTransform: aBlock. But it is not present, so I have two choices:

  1. Add it as extension method, but then it may clash if someone else has similar idea.
  2. Add private helper TowergameDelegate >> dict:at:ifPresentTransform:, which is longer and needs additional self receiver.

3. Contribute an implementation into Pharo, which bypasses the problem of clashing with someone else's package, when they have a similar need they'll benefit from it already been done.  That is assuming such would be a generally useful feature. It sounds like it to me, but could someone else support that?  Keeping in mind its good to encourage newcomers to step up to being a contributor and not just a user - expanding the circle of "Pharo Is Yours"

Note though, this will probably only be integrated to Pharo 7.  So you'll need your Baseline to conditional load it for Pharo 5/6 - but thats a good thing to practice anyway (someone else will need to advise how to go about it - I'm still getting up to speed on this)

cheers -ben
Reply | Threaded
Open this post in threaded view
|

Re: Stylistic question: private helper vs extension method?

Henrik Sperre Johansen
In reply to this post by Herby Vojčík

> On 8 Aug 2017, at 22:39 , Herby Vojčík <[hidden email]> wrote:
>
> Hello!
>
> I've got this portion in my delegate:
>
> requestPayload ifNotNil: [ uuidKeys do: [ :each |
> requestPayload at: each ifPresent: [ :s | requestPayload at: each put: (UUID fromString: s) ] ] ].
> responsePayload := self towergame clientSync: requestPayload.
> responsePayload ifNotNil: [ uuidKeys do: [ :each |
> responsePayload at: each ifPresent: [ :uuid | responsePayload at: each put: uuid asString ] ] ].
>
> Now I would gladly use something like Dictionary >> at:ifPresentTransform: aBlock. But it is not present, so I have two choices:
>
>  1. Add it as extension method, but then it may clash if someone else has similar idea.
>  2. Add private helper TowergameDelegate >> dict:at:ifPresentTransform:, which is longer and needs additional self receiver.
>
> Which is preferable?
>
> Herby
Following existing convention, an extension would be named #at:ifPresentPut:

I wonder though, wouldn't it be simpler to just put(UUID fromString: s) in the first place, and convert (dict at: uuidKey) in the accesses that needs it (or vice versa)?
Shifting the class of values like this seems like it could be a nightmare to debug and/or extend in the future remaining confident all uses will occur in the correct/expected order...

Cheers,
Henry

signature.asc (859 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Stylistic question: private helper vs extension method?

Esteban A. Maringolo
In reply to this post by EstebanLM
2017-08-08 17:49 GMT-03:00 Esteban Lorenzano <[hidden email]>:
> Extension. Helpers are for javaers.

Extensions are concise, and right to the point, but as long as they
stay in your private image or prove of value to everybody else both in
function and selector naming.

My guides are:
A. If it's your image, extend Dictionary, Object or whatever you want.
It's your mess, your image, your rules.
B. If you're going to share the code with others (as a library), then
you should check whether the selectors clash with existing ones, or
have "side effects".
C. If it's going in the "core" image, then the selector should be well
thought, having broad application semantics and coherence with the
existing selectors, like the current #at:ifAbsent:, #at:ifPresent:,
etc.

In this case I'd go for something like:

Dictionary>>#at: key ifPresentPut: aBlock
    "Lookup the given key in the receiver. If it is present, update it
with the value of evaluating the given block with the value associated
with the key. Otherwise, answer nil."

     ^self at: key ifPresent: [ :object | self at: key put: (aBlock
cull: object) ]


And you'd call it like:
  responsePayload at: each ifPresentPut: [ :uuid | uuid asString ].
or:
  responsePayload at: each ifPresentPut: [ 'FooBaz' ].


Regards,

Esteban A. Maringolo

Reply | Threaded
Open this post in threaded view
|

Re: Stylistic question: private helper vs extension method?

Herby Vojčík
Esteban A. Maringolo wrote:

> 2017-08-08 17:49 GMT-03:00 Esteban Lorenzano<[hidden email]>:
>> Extension. Helpers are for javaers.
>
> Extensions are concise, and right to the point, but as long as they
> stay in your private image or prove of value to everybody else both in
> function and selector naming.
>
> My guides are:
> A. If it's your image, extend Dictionary, Object or whatever you want.
> It's your mess, your image, your rules.\

True.

> B. If you're going to share the code with others (as a library), then
> you should check whether the selectors clash with existing ones, or
> have "side effects".
> C. If it's going in the "core" image, then the selector should be well
> thought, having broad application semantics and coherence with the
> existing selectors, like the current #at:ifAbsent:, #at:ifPresent:,
> etc.
>
> In this case I'd go for something like:
>
> Dictionary>>#at: key ifPresentPut: aBlock

Never imagined it should be at:ifPresentPut:. Not clear at the first
sight, but undobutedly it is the best pick, on the second one.

>      "Lookup the given key in the receiver. If it is present, update it
> with the value of evaluating the given block with the value associated
> with the key. Otherwise, answer nil."
>
>       ^self at: key ifPresent: [ :object | self at: key put: (aBlock
> cull: object) ]

Well, if it is seen as a thing of value, you or someone can add it to
pharo7 (I probably can not, not having and not wanting to have github
account). Maybe for Array / String as well.

> And you'd call it like:
>    responsePayload at: each ifPresentPut: [ :uuid | uuid asString ].

In my case, `responsePayload at: each ifPresentPut: #asString` probably.
I don't care for a bit of perf hit (I like Symbol >> value:, during
Amber development I often do `Smalltalk packages do: #commit` to commit
everything and see what changed in git diff).

> or:
>    responsePayload at: each ifPresentPut: [ 'FooBaz' ].
>
>
> Regards,
>
> Esteban A. Maringolo

Thanks, Herby