The Trunk: ToolBuilder-Kernel-pre.122.mcz

Am 24.04.2018 18:33:43 schrieb tim Rowledge <[hidden email]>:

This isn't a criticism of this particular bit of code, nor of Patrick BUT I really dislike the idiom of
a) providing simple setter/getter methods that not only allow but positively encourage people to write code that looks as nasty as C++. Objects are not C structures with field names.
b) when you do have a valid reason (and they certainly exist) for a setter and/or getter, naming them exactly as the inst var seems to me to just amplify the problem. Less problematic would be the style #setPathSettingSelector:

And yes, I'm sure I've written code that does the Bad Thing. That doesn't make it good.

> On 24-04-2018, at 1:29 AM, [hidden email] wrote:
>
> Patrick Rein uploaded a new version of ToolBuilder-Kernel to project The Trunk:
> http://source.squeak.org/trunk/ToolBuilder-Kernel-pre.122.mcz
>
> ==================== Summary ====================
>
> Name: ToolBuilder-Kernel-pre.122
> Author: pre
> Time: 24 April 2018, 10:29:02.215732 am
> UUID: 9e1d912d-13fb-7341-9aac-9ac6ebfb76ab
> Ancestors: ToolBuilder-Kernel-tpr.121
>
> Adds a callback to pluggable tree specifications to retrieve the currently selected path in the tree.
>
> =============== Diff against ToolBuilder-Kernel-tpr.121 ===============
>
> Item was changed:
> PluggableWidgetSpec subclass: #PluggableTreeSpec
> + instanceVariableNames: 'roots getSelectedPath setSelectedPath setSelected getSelected setSelectedParent getChildren hasChildren label icon unusedVar menu keyPress doubleClick dropItem dropAccept autoDeselect dragItem nodeClass columns vScrollBarPolicy hScrollBarPolicy dragStarted'
> - instanceVariableNames: 'roots getSelectedPath setSelected getSelected setSelectedParent getChildren hasChildren label icon unusedVar menu keyPress doubleClick dropItem dropAccept autoDeselect dragItem nodeClass columns vScrollBarPolicy hScrollBarPolicy dragStarted'
> classVariableNames: ''
> poolDictionaries: ''
> category: 'ToolBuilder-Kernel'!
>
> + !PluggableTreeSpec commentStamp: 'pre 4/24/2018 10:20' prior: 0!
> - !PluggableTreeSpec commentStamp: 'mvdg 3/21/2008 20:59' prior: 0!
> A pluggable tree widget. PluggableTrees are slightly different from lists in such that they ALWAYS store the actual objects and use the label selector to query for the label of the item. PluggableTrees also behave somewhat differently in such that they do not have a "getSelected" message but only a getSelectedPath message. The difference is that getSelectedPath is used to indicate by the model that the tree should select the appropriate path. This allows disambiguation of items. Because of this, implementations of PluggableTrees must always set their internal selection directly, e.g., rather than sending the model a setSelected message and wait for an update of the #getSelected the implementation must set the selection before sending the #setSelected message. If a client doesn't want this, it can always just signal a change of getSelectedPath to revert to whatever is needed.
>
> Instance variables:
> roots The message to retrieve the roots of the tree.
> getSelectedPath The message to retrieve the selected path in the tree.
> + setSelectedPath The message to set the selected path in the tree.
> setSelected The message to set the selected item in the tree.
> getChildren The message to retrieve the children of an item
> hasChildren The message to query for children of an item
> label The message to query for the label of an item.
> icon The message to query for the icon of an item.
> help The message to query for the help of an item.
> menu The message to query for the tree's menu
> keyPress The message to process a keystroke.
> wantsDrop The message to query whether a drop might be accepted.
> dropItem The message to drop an item.
> enableDrag Enable dragging from this tree.
> autoDeselect Whether the tree should allow automatic deselection or not.
> unusedVar (unused) This variable is a placeholder to fix problems with loading packages in 3.10.!
>
> Item was added:
> + ----- Method: PluggableTreeSpec>>setSelectedPath (in category 'accessing - selection') -----
> + setSelectedPath
> +
> + ^ setSelectedPath!
>
> Item was added:
> + ----- Method: PluggableTreeSpec>>setSelectedPath: (in category 'accessing - selection') -----
> + setSelectedPath: aSymbol
> +
> + setSelectedPath := aSymbol!
>
>
>


tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: DC: Divide and Conquer

Am 25.04.2018 07:37:58 schrieb Marcel Taeumel <[hidden email]>:

Hi Tim,

I think that such a discussion would be more fruitful if NOT "piggybacked" on top of a regular commit. Instead, collect some related commits, sum up the argument (which you did), and post it as a new thread with a meaningful title. Especially the collection of varying examples is a plus for such discussions to better understand the bigger picture.

Best,

Marcel

Am 24.04.2018 18:33:43 schrieb tim Rowledge <[hidden email]>:

This isn't a criticism of this particular bit of code, nor of Patrick BUT I really dislike the idiom of
a) providing simple setter/getter methods that not only allow but positively encourage people to write code that looks as nasty as C++. Objects are not C structures with field names.
b) when you do have a valid reason (and they certainly exist) for a setter and/or getter, naming them exactly as the inst var seems to me to just amplify the problem. Less problematic would be the style #setPathSettingSelector:

And yes, I'm sure I've written code that does the Bad Thing. That doesn't make it good.

> On 24-04-2018, at 1:29 AM, [hidden email] wrote:
>
> Patrick Rein uploaded a new version of ToolBuilder-Kernel to project The Trunk:
> http://source.squeak.org/trunk/ToolBuilder-Kernel-pre.122.mcz
>
> ==================== Summary ====================
>
> Name: ToolBuilder-Kernel-pre.122
> Author: pre
> Time: 24 April 2018, 10:29:02.215732 am
> UUID: 9e1d912d-13fb-7341-9aac-9ac6ebfb76ab
> Ancestors: ToolBuilder-Kernel-tpr.121
>
> Adds a callback to pluggable tree specifications to retrieve the currently selected path in the tree.
>
> =============== Diff against ToolBuilder-Kernel-tpr.121 ===============
>
> Item was changed:
> PluggableWidgetSpec subclass: #PluggableTreeSpec
> + instanceVariableNames: 'roots getSelectedPath setSelectedPath setSelected getSelected setSelectedParent getChildren hasChildren label icon unusedVar menu keyPress doubleClick dropItem dropAccept autoDeselect dragItem nodeClass columns vScrollBarPolicy hScrollBarPolicy dragStarted'
> - instanceVariableNames: 'roots getSelectedPath setSelected getSelected setSelectedParent getChildren hasChildren label icon unusedVar menu keyPress doubleClick dropItem dropAccept autoDeselect dragItem nodeClass columns vScrollBarPolicy hScrollBarPolicy dragStarted'
> classVariableNames: ''
> poolDictionaries: ''
> category: 'ToolBuilder-Kernel'!
>
> + !PluggableTreeSpec commentStamp: 'pre 4/24/2018 10:20' prior: 0!
> - !PluggableTreeSpec commentStamp: 'mvdg 3/21/2008 20:59' prior: 0!
> A pluggable tree widget. PluggableTrees are slightly different from lists in such that they ALWAYS store the actual objects and use the label selector to query for the label of the item. PluggableTrees also behave somewhat differently in such that they do not have a "getSelected" message but only a getSelectedPath message. The difference is that getSelectedPath is used to indicate by the model that the tree should select the appropriate path. This allows disambiguation of items. Because of this, implementations of PluggableTrees must always set their internal selection directly, e.g., rather than sending the model a setSelected message and wait for an update of the #getSelected the implementation must set the selection before sending the #setSelected message. If a client doesn't want this, it can always just signal a change of getSelectedPath to revert to whatever is needed.
>
> Instance variables:
> roots The message to retrieve the roots of the tree.
> getSelectedPath The message to retrieve the selected path in the tree.
> + setSelectedPath The message to set the selected path in the tree.
> setSelected The message to set the selected item in the tree.
> getChildren The message to retrieve the children of an item
> hasChildren The message to query for children of an item
> label The message to query for the label of an item.
> icon The message to query for the icon of an item.
> help The message to query for the help of an item.
> menu The message to query for the tree's menu
> keyPress The message to process a keystroke.
> wantsDrop The message to query whether a drop might be accepted.
> dropItem The message to drop an item.
> enableDrag Enable dragging from this tree.
> autoDeselect Whether the tree should allow automatic deselection or not.
> unusedVar (unused) This variable is a placeholder to fix problems with loading packages in 3.10.!
>
> Item was added:
> + ----- Method: PluggableTreeSpec>>setSelectedPath (in category 'accessing - selection') -----
> + setSelectedPath
> +
> + ^ setSelectedPath!
>
> Item was added:
> + ----- Method: PluggableTreeSpec>>setSelectedPath: (in category 'accessing - selection') -----
> + setSelectedPath: aSymbol
> +
> + setSelectedPath := aSymbol!
>
>
>


tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: DC: Divide and Conquer

> On 25-04-2018, at 10:29 AM, Levente Uzonyi wrote: > > Why would you want to create private accessors when you can just access the variable directly? > Creating accessors for a variable that should not be accessed externally is bad practice IMHO. Exactly my point. The plausible reason for using a message to get an instvar is that there *might* be some testing for is nil, or empty etc that makes your code nicer - no seemingly random chunks of code referring to an ivar and having to test it , maybe incorrectly, scattered around. You can make an argument (not completely sure I'd agree with it) that using a message send as your normal style makes this cleaner - for plain instvars you just return them, for ones needing testing you do the testing etc. I guess it's true that having code where in some places you use the direct access and in others you have to use a message in order to make sure the ivar is set up correctly is messy and requires some detailed knowledge of the class. Hmm, yeah, maybe. The downside is the whole bad-C-programmer thing. We could probably do something terribly clever and make the compiler understand that if there is a method named appropriately (simplest choice is "the same as the variable name" but we could set any rule we want) then any place where the direct bytecode would normally be used is replaced by a magic bytecode that actually sends the message. It might even be something the Sista could do magic with. This likely-insane idea would make our code look the same but allow adding a "test and initialise etc" method that just gets used if needed. All this is just fluff though by comparison to not liking raw getter/setter methods being public. tim -- tim Rowledge; [hidden email]; http://www.rowledge.org/tim A)bort, R)etry, I)nfluence with large hammer.