Christoph Thiede uploaded a new version of Tools to project The Inbox:
http://source.squeak.org/inbox/Tools-ct.944.mcz ==================== Summary ==================== Name: Tools-ct.944 Author: ct Time: 23 February 2020, 5:50:43.489831 pm UUID: 0c54eef3-3e82-1340-9426-3e94151157d6 Ancestors: Tools-mt.940 Fixes a bug/unnecessary limitation in VersionsBrowser class >> #browseMethod: that raised an error when browsing a method that had been removed from the system. You can also reproduce it via: thisContext method browse "on a fresh image". =============== Diff against Tools-mt.940 =============== Item was changed: ----- Method: VersionsBrowser class>>browseMethod: (in category 'instance creation') ----- browseMethod: aCompiledMethod + ^ (self browseVersionsOf: aCompiledMethod) + ifNil: [^ nil]; - ^ (self browseVersionsForClass: aCompiledMethod methodClass selector: aCompiledMethod selector) selectMethod: aCompiledMethod; yourself! Item was added: + ----- Method: VersionsBrowser class>>browseVersionsOf: (in category 'instance creation') ----- + browseVersionsOf: aCompiledMethod + + | methodClass methodSelector | + methodClass := aCompiledMethod methodClass. + methodSelector := aCompiledMethod selector. + ^ self + browseVersionsOf: aCompiledMethod + class: methodClass + meta: methodClass isMeta + category: (methodClass organization categoryOfElement: methodSelector) + selector: methodSelector! |
How about not adding #browseVersionsOf:, but chaging the
body of #browseMethod: to what #browseVersionsOf: is below? ^x ifNil: [ ^nil ] is the same as ^x but longer, isn't it? Levente On Sun, 23 Feb 2020, [hidden email] wrote: > Christoph Thiede uploaded a new version of Tools to project The Inbox: > http://source.squeak.org/inbox/Tools-ct.944.mcz > > ==================== Summary ==================== > > Name: Tools-ct.944 > Author: ct > Time: 23 February 2020, 5:50:43.489831 pm > UUID: 0c54eef3-3e82-1340-9426-3e94151157d6 > Ancestors: Tools-mt.940 > > Fixes a bug/unnecessary limitation in VersionsBrowser class >> #browseMethod: that raised an error when browsing a method that had been removed from the system. > > You can also reproduce it via: thisContext method browse "on a fresh image". > > =============== Diff against Tools-mt.940 =============== > > Item was changed: > ----- Method: VersionsBrowser class>>browseMethod: (in category 'instance creation') ----- > browseMethod: aCompiledMethod > > + ^ (self browseVersionsOf: aCompiledMethod) > + ifNil: [^ nil]; > - ^ (self browseVersionsForClass: aCompiledMethod methodClass selector: aCompiledMethod selector) > selectMethod: aCompiledMethod; > yourself! > > Item was added: > + ----- Method: VersionsBrowser class>>browseVersionsOf: (in category 'instance creation') ----- > + browseVersionsOf: aCompiledMethod > + > + | methodClass methodSelector | > + methodClass := aCompiledMethod methodClass. > + methodSelector := aCompiledMethod selector. > + ^ self > + browseVersionsOf: aCompiledMethod > + class: methodClass > + meta: methodClass isMeta > + category: (methodClass organization categoryOfElement: methodSelector) > + selector: methodSelector! |
The intended difference is that #browseVersionsOf: opens a VB on all versions of aCompiledMethod, whereas #browseMethod: selects aCompiledMethod by default. This is what #selectMethod: does.
Do you think the initial selection does not matter?
Best, Christoph Von: Squeak-dev <[hidden email]> im Auftrag von Levente Uzonyi <[hidden email]>
Gesendet: Sonntag, 23. Februar 2020 19:40:47 An: [hidden email] Betreff: Re: [squeak-dev] The Inbox: Tools-ct.944.mcz How about not adding #browseVersionsOf:, but chaging the
body of #browseMethod: to what #browseVersionsOf: is below? ^x ifNil: [ ^nil ] is the same as ^x but longer, isn't it? Levente On Sun, 23 Feb 2020, [hidden email] wrote: > Christoph Thiede uploaded a new version of Tools to project The Inbox: > http://source.squeak.org/inbox/Tools-ct.944.mcz > > ==================== Summary ==================== > > Name: Tools-ct.944 > Author: ct > Time: 23 February 2020, 5:50:43.489831 pm > UUID: 0c54eef3-3e82-1340-9426-3e94151157d6 > Ancestors: Tools-mt.940 > > Fixes a bug/unnecessary limitation in VersionsBrowser class >> #browseMethod: that raised an error when browsing a method that had been removed from the system. > > You can also reproduce it via: thisContext method browse "on a fresh image". > > =============== Diff against Tools-mt.940 =============== > > Item was changed: > ----- Method: VersionsBrowser class>>browseMethod: (in category 'instance creation') ----- > browseMethod: aCompiledMethod > > + ^ (self browseVersionsOf: aCompiledMethod) > + ifNil: [^ nil]; > - ^ (self browseVersionsForClass: aCompiledMethod methodClass selector: aCompiledMethod selector) > selectMethod: aCompiledMethod; > yourself! > > Item was added: > + ----- Method: VersionsBrowser class>>browseVersionsOf: (in category 'instance creation') ----- > + browseVersionsOf: aCompiledMethod > + > + | methodClass methodSelector | > + methodClass := aCompiledMethod methodClass. > + methodSelector := aCompiledMethod selector. > + ^ self > + browseVersionsOf: aCompiledMethod > + class: methodClass > + meta: methodClass isMeta > + category: (methodClass organization categoryOfElement: methodSelector) > + selector: methodSelector!
Carpe Squeak!
|
My bad, I didn't notice there was no - at the beginning of 2 lines in the
diff, so ignore what I wrote. But, the rather unusual ifNil: didn't help noticing that either. I really think #ifNotNil: should be used there instead: ^(...) ifNotNil: [ ... ] Levente On Sun, 23 Feb 2020, Thiede, Christoph wrote: > > The intended difference is that #browseVersionsOf: opens a VB on all versions of aCompiledMethod, whereas #browseMethod: selects aCompiledMethod by default. This is what #selectMethod: does. > > > Do you think the initial selection does not matter? > > > Best, > > Christoph > > _________________________________________________________________________________________________________________________________________________________________________________________________________________________________ > Von: Squeak-dev <[hidden email]> im Auftrag von Levente Uzonyi <[hidden email]> > Gesendet: Sonntag, 23. Februar 2020 19:40:47 > An: [hidden email] > Betreff: Re: [squeak-dev] The Inbox: Tools-ct.944.mcz > How about not adding #browseVersionsOf:, but chaging the > body of #browseMethod: to what #browseVersionsOf: is below? > > ^x ifNil: [ ^nil ] is the same as ^x but longer, isn't it? > > > Levente > > On Sun, 23 Feb 2020, [hidden email] wrote: > > > Christoph Thiede uploaded a new version of Tools to project The Inbox: > > http://source.squeak.org/inbox/Tools-ct.944.mcz > > > > ==================== Summary ==================== > > > > Name: Tools-ct.944 > > Author: ct > > Time: 23 February 2020, 5:50:43.489831 pm > > UUID: 0c54eef3-3e82-1340-9426-3e94151157d6 > > Ancestors: Tools-mt.940 > > > > Fixes a bug/unnecessary limitation in VersionsBrowser class >> #browseMethod: that raised an error when browsing a method that had been removed from the system. > > > > You can also reproduce it via: thisContext method browse "on a fresh image". > > > > =============== Diff against Tools-mt.940 =============== > > > > Item was changed: > > ----- Method: VersionsBrowser class>>browseMethod: (in category 'instance creation') ----- > > browseMethod: aCompiledMethod > > > > + ^ (self browseVersionsOf: aCompiledMethod) > > + ifNil: [^ nil]; > > - ^ (self browseVersionsForClass: aCompiledMethod methodClass selector: aCompiledMethod selector) > > selectMethod: aCompiledMethod; > > yourself! > > > > Item was added: > > + ----- Method: VersionsBrowser class>>browseVersionsOf: (in category 'instance creation') ----- > > + browseVersionsOf: aCompiledMethod > > + > > + | methodClass methodSelector | > > + methodClass := aCompiledMethod methodClass. > > + methodSelector := aCompiledMethod selector. > > + ^ self > > + browseVersionsOf: aCompiledMethod > > + class: methodClass > > + meta: methodClass isMeta > > + category: (methodClass organization categoryOfElement: methodSelector) > > + selector: methodSelector! > > > |
I guess this is a matter of taste again. :-)
I always try to handle all edge cases first, mostly by returning self or something other, so I can keep the main logic clean without any nestings. Not sure whether this is a pattern, but I believe this often helps to improve readability :) In this example, nil is an edge case, so I would rather tend to handle it first, instead of introducing an extra temporary variable for handling the most common "expected" scenario.
Best, Christoph Von: Squeak-dev <[hidden email]> im Auftrag von Levente Uzonyi <[hidden email]>
Gesendet: Sonntag, 23. Februar 2020 23:46:57 An: The general-purpose Squeak developers list Betreff: Re: [squeak-dev] The Inbox: Tools-ct.944.mcz My bad, I didn't notice there was no - at the beginning of 2 lines in the
diff, so ignore what I wrote. But, the rather unusual ifNil: didn't help noticing that either. I really think #ifNotNil: should be used there instead: ^(...) ifNotNil: [ ... ] Levente On Sun, 23 Feb 2020, Thiede, Christoph wrote: > > The intended difference is that #browseVersionsOf: opens a VB on all versions of aCompiledMethod, whereas #browseMethod: selects aCompiledMethod by default. This is what #selectMethod: does. > > > Do you think the initial selection does not matter? > > > Best, > > Christoph > > _________________________________________________________________________________________________________________________________________________________________________________________________________________________________ > Von: Squeak-dev <[hidden email]> im Auftrag von Levente Uzonyi <[hidden email]> > Gesendet: Sonntag, 23. Februar 2020 19:40:47 > An: [hidden email] > Betreff: Re: [squeak-dev] The Inbox: Tools-ct.944.mcz > How about not adding #browseVersionsOf:, but chaging the > body of #browseMethod: to what #browseVersionsOf: is below? > > ^x ifNil: [ ^nil ] is the same as ^x but longer, isn't it? > > > Levente > > On Sun, 23 Feb 2020, [hidden email] wrote: > > > Christoph Thiede uploaded a new version of Tools to project The Inbox: > > http://source.squeak.org/inbox/Tools-ct.944.mcz > > > > ==================== Summary ==================== > > > > Name: Tools-ct.944 > > Author: ct > > Time: 23 February 2020, 5:50:43.489831 pm > > UUID: 0c54eef3-3e82-1340-9426-3e94151157d6 > > Ancestors: Tools-mt.940 > > > > Fixes a bug/unnecessary limitation in VersionsBrowser class >> #browseMethod: that raised an error when browsing a method that had been removed from the system. > > > > You can also reproduce it via: thisContext method browse "on a fresh image". > > > > =============== Diff against Tools-mt.940 =============== > > > > Item was changed: > > ----- Method: VersionsBrowser class>>browseMethod: (in category 'instance creation') ----- > > browseMethod: aCompiledMethod > > > > + ^ (self browseVersionsOf: aCompiledMethod) > > + ifNil: [^ nil]; > > - ^ (self browseVersionsForClass: aCompiledMethod methodClass selector: aCompiledMethod selector) > > selectMethod: aCompiledMethod; > > yourself! > > > > Item was added: > > + ----- Method: VersionsBrowser class>>browseVersionsOf: (in category 'instance creation') ----- > > + browseVersionsOf: aCompiledMethod > > + > > + | methodClass methodSelector | > > + methodClass := aCompiledMethod methodClass. > > + methodSelector := aCompiledMethod selector. > > + ^ self > > + browseVersionsOf: aCompiledMethod > > + class: methodClass > > + meta: methodClass isMeta > > + category: (methodClass organization categoryOfElement: methodSelector) > > + selector: methodSelector! > > >
Carpe Squeak!
|
Am So., 23. Feb. 2020 um 23:55 Uhr schrieb Thiede, Christoph <[hidden email]>:
It's an idiom called "Guard Clause" |
> It's an idiom called "Guard Clause"
Thank you :-) Von: Squeak-dev <[hidden email]> im Auftrag von Jakob Reschke <[hidden email]>
Gesendet: Sonntag, 23. Februar 2020 23:58:13 An: The general-purpose Squeak developers list Betreff: Re: [squeak-dev] The Inbox: Tools-ct.944.mcz Am So., 23. Feb. 2020 um 23:55 Uhr schrieb Thiede, Christoph <[hidden email]>:
It's an idiom called "Guard Clause"
Carpe Squeak!
|
In reply to this post by Christoph Thiede
On Sun, 23 Feb 2020, Thiede, Christoph wrote:
> > I guess this is a matter of taste again. :-) > > > I always try to handle all edge cases first, mostly by returning self or something other, so I can keep the main logic clean without any nestings. Not sure whether this is a pattern, but I believe this often helps to improve > readability :) > > In this example, nil is an edge case, so I would rather tend to handle it first, instead of introducing an extra temporary variable for handling the most common "expected" scenario. Um, how is what I suggested not handling nil first? Levente > > > Best, > > Christoph > > _________________________________________________________________________________________________________________________________________________________________________________________________________________________________ > Von: Squeak-dev <[hidden email]> im Auftrag von Levente Uzonyi <[hidden email]> > Gesendet: Sonntag, 23. Februar 2020 23:46:57 > An: The general-purpose Squeak developers list > Betreff: Re: [squeak-dev] The Inbox: Tools-ct.944.mcz > My bad, I didn't notice there was no - at the beginning of 2 lines in the > diff, so ignore what I wrote. > But, the rather unusual ifNil: didn't help noticing that either. I really > think #ifNotNil: should be used there instead: > > ^(...) ifNotNil: [ ... ] > > > Levente > > On Sun, 23 Feb 2020, Thiede, Christoph wrote: > > > > > The intended difference is that #browseVersionsOf: opens a VB on all versions of aCompiledMethod, whereas #browseMethod: selects aCompiledMethod by default. This is what #selectMethod: does. > > > > > > Do you think the initial selection does not matter? > > > > > > Best, > > > > Christoph > > > >________________________________________________________________________________________________________________________________________________________________________________________________________________________________ > _ > > Von: Squeak-dev <[hidden email]> im Auftrag von Levente Uzonyi <[hidden email]> > > Gesendet: Sonntag, 23. Februar 2020 19:40:47 > > An: [hidden email] > > Betreff: Re: [squeak-dev] The Inbox: Tools-ct.944.mcz > > How about not adding #browseVersionsOf:, but chaging the > > body of #browseMethod: to what #browseVersionsOf: is below? > > > > ^x ifNil: [ ^nil ] is the same as ^x but longer, isn't it? > > > > > > Levente > > > > On Sun, 23 Feb 2020, [hidden email] wrote: > > > > > Christoph Thiede uploaded a new version of Tools to project The Inbox: > > > http://source.squeak.org/inbox/Tools-ct.944.mcz > > > > > > ==================== Summary ==================== > > > > > > Name: Tools-ct.944 > > > Author: ct > > > Time: 23 February 2020, 5:50:43.489831 pm > > > UUID: 0c54eef3-3e82-1340-9426-3e94151157d6 > > > Ancestors: Tools-mt.940 > > > > > > Fixes a bug/unnecessary limitation in VersionsBrowser class >> #browseMethod: that raised an error when browsing a method that had been removed from the system. > > > > > > You can also reproduce it via: thisContext method browse "on a fresh image". > > > > > > =============== Diff against Tools-mt.940 =============== > > > > > > Item was changed: > > > ----- Method: VersionsBrowser class>>browseMethod: (in category 'instance creation') ----- > > > browseMethod: aCompiledMethod > > > > > > + ^ (self browseVersionsOf: aCompiledMethod) > > > + ifNil: [^ nil]; > > > - ^ (self browseVersionsForClass: aCompiledMethod methodClass selector: aCompiledMethod selector) > > > selectMethod: aCompiledMethod; > > > yourself! > > > > > > Item was added: > > > + ----- Method: VersionsBrowser class>>browseVersionsOf: (in category 'instance creation') ----- > > > + browseVersionsOf: aCompiledMethod > > > + > > > + | methodClass methodSelector | > > > + methodClass := aCompiledMethod methodClass. > > > + methodSelector := aCompiledMethod selector. > > > + ^ self > > > + browseVersionsOf: aCompiledMethod > > > + class: methodClass > > > + meta: methodClass isMeta > > > + category: (methodClass organization categoryOfElement: methodSelector) > > > + selector: methodSelector! > > > > > > > > |
Free forum by Nabble | Edit this page |