RemotesManager Fishiness

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

RemotesManager Fishiness

Sean P. DeNigris
Administrator
[rant]
This is a weird class! It's protocol seems convoluted to me (even though it only contains one operation!).

The standard dialog #actionBlock: is eschewed for #selectedChangedBlock:, which lets you know every time an item is checked or unchecked. I'm not clear on the use case for that verbose level of updating. I would think you'd mostly be interested in the final state once the dialog was accepted.

In addition, there is a #selectedRemotes instVar, but it never gets updated and has no getter.

Maybe I'm just not getting it, but it seems like something was quickly hacked together as an experiment to serve an immediate purpose without much design thought. While I love new features, this worries me because it sounds very familiar and three forks may be one too many ;) Maybe we should have a more formal code review procedure for shiny new features (non-bug fix/enhancement)?
[/rant]

14303 RemotesManager: Make more generally useful
https://pharo.fogbugz.com/default.asp?14303
Initial slice incoming...
Cheers,
Sean
Reply | Threaded
Open this post in threaded view
|

Re: RemotesManager Fishiness

Sean P. DeNigris
Administrator
Sean P. DeNigris wrote
Initial slice incoming...
Fix in inbox: SLICE-Issue-14303-RemotesManager-Make-more-generally-useful-SeanDeNigris.1

RemotesManager:
- Properly update selectedRemotes instVar
- Add #selectedRemotes getter

Now you can e.g.:
        repoManager := RemotesManager new.
        repoManager openDialogWithSpec okAction: [ repoManager selectedRemotes ]
Cheers,
Sean
Reply | Threaded
Open this post in threaded view
|

Re: RemotesManager Fishiness

stepharo
In reply to this post by Sean P. DeNigris
I completely agree with you and one of the problems is that some people
did not take well
reviews of their code. I hope that we will be able to change this spirit.

> [rant]
> This is a weird class! It's protocol seems convoluted to me (even though it
> only contains one operation!).
>
> The standard dialog #actionBlock: is eschewed for #selectedChangedBlock:,
> which lets you know every time an item is checked or unchecked. I'm not
> clear on the use case for that verbose level of updating. I would think
> you'd mostly be interested in the final state once the dialog was accepted.
>
> In addition, there is a #selectedRemotes instVar, but it never gets updated
> and has no getter.
>
> Maybe I'm just not getting it, but it seems like something was quickly
> hacked together as an experiment to serve an immediate purpose without much
> design thought. While I love new features, this worries me because it sounds
> very familiar and three forks may be one too many ;) Maybe we should have a
> more formal code review procedure for shiny new features (non-bug
> fix/enhancement)?
> [/rant]
>
> 14303 RemotesManager: Make more generally useful
> https://pharo.fogbugz.com/default.asp?14303
> Initial slice incoming...
>
>
>
> -----
> Cheers,
> Sean
> --
> View this message in context: http://forum.world.st/RemotesManager-Fishiness-tp4786315.html
> Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
>
>