Smalltalk › Pharo › Pharo Smalltalk Developers

Generate accessors refactoring

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

29 messages Options

stepharong

Re: Generate accessors refactoring

> As for my “real experience”, I wrote the accessor refactoring over 22
> years ago. Since then, I’ve had several times that it has saved me from
> overriding a method that I didn’t want to be overridden. As for saving
> me from destroying the system, it most likely has, but not in the way
> you probably expect. When I would test the refactorings, I would
> purposely try to destroy the system by doing dangerous things. If the
> system crashed, I would fix the refactorings, or add an explanation that
> the refactoring didn’t/couldn’t check for that behavior. Testing in this
> way gave us confidence when giving demos. We could allow people to yell
> out refactorings that they wanted to see performed and we were fairly
> confident that everything would keep running. Over the years, we renamed
> Object to Thing, True/False to False/True,

I would love to try these three ;)

#+ to #p:, extracted/inlined
> bunches of code in String and OrderedCollection, etc. Only once in all
> of the demos did we destroy the system. We renamed a method that was
> being used by the process scheduler. We don’t try to fix running code
> with the new names so when the original method was removed, the process
> scheduler threw an error and crashed the image.

This is a cool example.
Pablo is working on updating instances during a refactoring and we should
try.

We could have had the
> rename method refactoring handle this case also, but that would have
> required some non-portable code.
>
>
> John Brant

--
Using Opera's mail client: http://www.opera.com/mail/

Denis Kudriashov

Re: Generate accessors refactoring

In reply to this post by John Brant-2

2016-12-07 18:00 GMT+01:00 John Brant <[hidden email]>:

Which of these would you consider valid setter methods for foo?

I understand pretty well what you are trying to explain. I just think that generation of methods like #instVar1/#instVar1: is useless. It always break user control flow. Simple generator should just skip existing accessors according to "name definition" of accessors (methods with #instVar name). Besides skipping them not makes refactoring unsafe.

Uko2

Re: Generate accessors refactoring

In reply to this post by John Brant-2

On 7 Dec 2016, at 18:00, John Brant <[hidden email]> wrote:

Since then, I’ve had several times that it has saved me from overriding a method that I didn’t want to be overridden.

Was this done by creating a method with a different name or by letting you know that the method already exists and asking what you want to do next?

Uko

Nicolas Passerini

Re: Generate accessors refactoring

In reply to this post by Ben Coman

Let me recapitulate a little.

A) What to do in case that a method with same selector already exists in the same class?

So far I think we've come up with four ideas:

1. Create with an alternative name such as instVar1 / instVar1:

2. Create with an alternative name but ask the user what the name should be.

3. Replace existing method with simple accessor.

4. Do nothing.

I think that the last one is the best alternative and should be the default action. Let me analyze the others:

1. Is the current behavior, but I always end up deleting the method, I can't think of a situation in which I want to keep a method named #name1 .

I know that there was a lot of thinking on top of this ideas, but even so I think that we can (should) rethink things once in a while.

2. A little better, because lets me select a name better than #name1. Still I do not like it, because this means that I have a Class with

- an instance variable 'name'

- a method #name which is not the accessor for 'name' inst var.

- and an accessor which is named in another way

I think this can happen in two situations:

- The original #name method is in fact a (smarter) accessor (i.e., for example it provides a lazy initialization). In this case the best action is 4 => do nothing, accessor already exists, do not provide a new one.

- The original #name does something else, that maybe has nothing to do with the variable name... in this case, I think the best is aborting the refactor. I am not proposing that refactoring browser should abort the refactor automatically, probably not, but that is what I would do as user: my class has a weird name clash, so I should rename either the inst var or the method, it has no automatic solution.

I am not sure about how the "generate accessors" could help me coping with name clashes, but I do not like generating accessor with another name different from the inst var name is a good idea. We could have this as an option to the user, but should not be the default.

3. This is really dangerous, we shouldn't do it. I am not sure if someone would like to have this as a (non default) option.

B) What if the colliding method is in a superclass?

Well, pretty much the same but:

- in option 3 instead of replacing a method you could override it.

- the method in the superclass could not be a "smarter accessor" unless you are trying to create accessors in subclass for an inst var in the superclass... I do not like it. So I'd consider it allways as a name clash.

Then I think that the best option is still do nothing (option 4). You could offer to create method with another name as alternative (option 2)... but I still think that this kind of name "coincidences" may introduce bugs in the future and should be avoided.

C) Colliding methods in subclasses.

Here we have a more subtle situation because the existing method could be a smarter accessor, that even could have its motivation to be there (for example because initialization code only is useful for one subclass).

I still think that doing nothing could be a safe solution, better than creating it with an alternative name (but we could allow it also).

Here we could think also about writing the accessor, because it will not override preexistent behavior. But we have to think about a few situations:

- Easy, if any of the colliding methods (they could be serveral one for each subclass) is not a smarter accessor... we have name clashes and we cannot do anything.

- If all colliding methods are smart accessors, we could create one in the superclass, provided that there exists at least one subclass which does not provide its own implementation.

Of course there is not precise way of determining if a preexistent method can be considered as a smarter accessor or not... so we only can let the user decide.

To resume:

- Doing nothing will be the best default as it is suitable for all cases, and can not do any harm.

- Creating methods with alternative names could be optional, but shouldn't be default. In this case I prefer to ask the user for a name instead of creating one automatically.

- Creating the method even in case of collision can be dangerous... I am not sure if we should even allow it as an option.

2016-12-07 0:03 GMT+01:00 Ben Coman <[hidden email]>:

How about mostly keeping the existing behaviour, but default to
collisions being deselected?
Then at least you don't need to search for it.
It minimises the chance of missing it by mistake.
The deselected item easily stands out to be reselected if required.

P.S. An additional side idea: If collision is from a superclass then
append (Superclass>>name) to the list item, and clicking on it shows a
code dialog to the right similar to Spotter code review pane, to make
a review easier.

cheers -ben

On Tue, Dec 6, 2016 at 10:09 PM, Yuriy Tymchuk <[hidden email]> wrote:
> It shouldn’t be too intelligent. Keep the default behavior, and for each
> method add a 3-state checkbox: create (default), skip, force (override).
> Also having a usage recorder would be nice to know if developers are
> actually checking the other state more often than the default one.
>
> Uko
>
> On 6 Dec 2016, at 13:03, Thierry Goubier <[hidden email]> wrote:
>
>
>
> 2016-12-06 11:34 GMT+01:00 Denis Kudriashov <[hidden email]>:
>>
>>
>> 2016-12-06 11:28 GMT+01:00 Yuriy Tymchuk <[hidden email]>:
>>>
>>> Additionally it was annoying if a superclass has a method with the same
>>> name. For example if you have a name var and you create accessors I’d like
>>> to have actually a ’name’ getter and not ’name1’
>>
>>
>> Yes
>>
>
> If you start to make it so intelligent it requires half a dozen click to
> generate a simple accessor, then people will stop using the refactoring and
> write the code by hand...
>
> Thierry
>
>
>

Eliot Miranda-2

Re: Generate accessors refactoring

Hi Nicolas,

On Dec 8, 2016, at 1:16 AM, Nicolas Passerini <[hidden email]> wrote:

Let me recapitulate a little.
A) What to do in case that a method with same selector already exists in the same class?
So far I think we've come up with four ideas:
1. Create with an alternative name such as instVar1 / instVar1:
2. Create with an alternative name but ask the user what the name should be.
3. Replace existing method with simple accessor.
4. Do nothing.

To be clear, you don't mean Do Nothing; you mean Issue an Error and Abort the Refactoring right?

I think that the last one is the best alternative and should be the default action. Let me analyze the others:
1. Is the current behavior, but I always end up deleting the method, I can't think of a situation in which I want to keep a method named #name1 .
I know that there was a lot of thinking on top of this ideas, but even so I think that we can (should) rethink things once in a while.

2. A little better, because lets me select a name better than #name1. Still I do not like it, because this means that I have a Class with
- an instance variable 'name'
- a method #name which is not the accessor for 'name' inst var.
- and an accessor which is named in another way

I think this can happen in two situations:
- The original #name method is in fact a (smarter) accessor (i.e., for example it provides a lazy initialization). In this case the best action is 4 => do nothing, accessor already exists, do not provide a new one.
- The original #name does something else, that maybe has nothing to do with the variable name... in this case, I think the best is aborting the refactor. I am not proposing that refactoring browser should abort the refactor automatically, probably not, but that is what I would do as user: my class has a weird name clash, so I should rename either the inst var or the method, it has no automatic solution.

I am not sure about how the "generate accessors" could help me coping with name clashes, but I do not like generating accessor with another name different from the inst var name is a good idea. We could have this as an option to the user, but should not be the default.

3. This is really dangerous, we shouldn't do it. I am not sure if someone would like to have this as a (non default) option.

B) What if the colliding method is in a superclass?
Well, pretty much the same but:
- in option 3 instead of replacing a method you could override it.
- the method in the superclass could not be a "smarter accessor" unless you are trying to create accessors in subclass for an inst var in the superclass... I do not like it. So I'd consider it allways as a name clash.

Then I think that the best option is still do nothing (option 4). You could offer to create method with another name as alternative (option 2)... but I still think that this kind of name "coincidences" may introduce bugs in the future and should be avoided.

C) Colliding methods in subclasses.
Here we have a more subtle situation because the existing method could be a smarter accessor, that even could have its motivation to be there (for example because initialization code only is useful for one subclass).

I still think that doing nothing could be a safe solution, better than creating it with an alternative name (but we could allow it also).

Here we could think also about writing the accessor, because it will not override preexistent behavior. But we have to think about a few situations:
- Easy, if any of the colliding methods (they could be serveral one for each subclass) is not a smarter accessor... we have name clashes and we cannot do anything.
- If all colliding methods are smart accessors, we could create one in the superclass, provided that there exists at least one subclass which does not provide its own implementation.

Of course there is not precise way of determining if a preexistent method can be considered as a smarter accessor or not... so we only can let the user decide.

To resume:
- Doing nothing will be the best default as it is suitable for all cases, and can not do any harm.
- Creating methods with alternative names could be optional, but shouldn't be default. In this case I prefer to ask the user for a name instead of creating one automatically.
- Creating the method even in case of collision can be dangerous... I am not sure if we should even allow it as an option.

2016-12-07 0:03 GMT+01:00 Ben Coman <[hidden email]>:
How about mostly keeping the existing behaviour, but default to
collisions being deselected?
Then at least you don't need to search for it.
It minimises the chance of missing it by mistake.
The deselected item easily stands out to be reselected if required.

P.S. An additional side idea: If collision is from a superclass then
append (Superclass>>name) to the list item, and clicking on it shows a
code dialog to the right similar to Spotter code review pane, to make
a review easier.

cheers -ben

On Tue, Dec 6, 2016 at 10:09 PM, Yuriy Tymchuk <[hidden email]> wrote:
> It shouldn’t be too intelligent. Keep the default behavior, and for each
> method add a 3-state checkbox: create (default), skip, force (override).
> Also having a usage recorder would be nice to know if developers are
> actually checking the other state more often than the default one.
>
> Uko
>
> On 6 Dec 2016, at 13:03, Thierry Goubier <[hidden email]> wrote:
>
>
>
> 2016-12-06 11:34 GMT+01:00 Denis Kudriashov <[hidden email]>:
>>
>>
>> 2016-12-06 11:28 GMT+01:00 Yuriy Tymchuk <[hidden email]>:
>>>
>>> Additionally it was annoying if a superclass has a method with the same
>>> name. For example if you have a name var and you create accessors I’d like
>>> to have actually a ’name’ getter and not ’name1’
>>
>>
>> Yes
>>
>
> If you start to make it so intelligent it requires half a dozen click to
> generate a simple accessor, then people will stop using the refactoring and
> write the code by hand...
>
> Thierry
>
>
>

Aliaksei Syrel

Re: Generate accessors refactoring

Could we deselect check box by default in Nautilus if refactoring suggests instVar1 / instVar1?

Every time have to go through the list and deselect...

Cheers,

Alex

On 8 December 2016 at 14:11, Eliot Miranda <[hidden email]> wrote:

Hi Nicolas,

On Dec 8, 2016, at 1:16 AM, Nicolas Passerini <[hidden email]> wrote:

Let me recapitulate a little.
A) What to do in case that a method with same selector already exists in the same class?
So far I think we've come up with four ideas:
1. Create with an alternative name such as instVar1 / instVar1:
2. Create with an alternative name but ask the user what the name should be.
3. Replace existing method with simple accessor.
4. Do nothing.

To be clear, you don't mean Do Nothing; you mean Issue an Error and Abort the Refactoring right?

I think that the last one is the best alternative and should be the default action. Let me analyze the others:
1. Is the current behavior, but I always end up deleting the method, I can't think of a situation in which I want to keep a method named #name1 .
I know that there was a lot of thinking on top of this ideas, but even so I think that we can (should) rethink things once in a while.

2. A little better, because lets me select a name better than #name1. Still I do not like it, because this means that I have a Class with
- an instance variable 'name'
- a method #name which is not the accessor for 'name' inst var.
- and an accessor which is named in another way

I think this can happen in two situations:
- The original #name method is in fact a (smarter) accessor (i.e., for example it provides a lazy initialization). In this case the best action is 4 => do nothing, accessor already exists, do not provide a new one.
- The original #name does something else, that maybe has nothing to do with the variable name... in this case, I think the best is aborting the refactor. I am not proposing that refactoring browser should abort the refactor automatically, probably not, but that is what I would do as user: my class has a weird name clash, so I should rename either the inst var or the method, it has no automatic solution.

I am not sure about how the "generate accessors" could help me coping with name clashes, but I do not like generating accessor with another name different from the inst var name is a good idea. We could have this as an option to the user, but should not be the default.

3. This is really dangerous, we shouldn't do it. I am not sure if someone would like to have this as a (non default) option.

B) What if the colliding method is in a superclass?
Well, pretty much the same but:
- in option 3 instead of replacing a method you could override it.
- the method in the superclass could not be a "smarter accessor" unless you are trying to create accessors in subclass for an inst var in the superclass... I do not like it. So I'd consider it allways as a name clash.

Then I think that the best option is still do nothing (option 4). You could offer to create method with another name as alternative (option 2)... but I still think that this kind of name "coincidences" may introduce bugs in the future and should be avoided.

C) Colliding methods in subclasses.
Here we have a more subtle situation because the existing method could be a smarter accessor, that even could have its motivation to be there (for example because initialization code only is useful for one subclass).

I still think that doing nothing could be a safe solution, better than creating it with an alternative name (but we could allow it also).

Here we could think also about writing the accessor, because it will not override preexistent behavior. But we have to think about a few situations:
- Easy, if any of the colliding methods (they could be serveral one for each subclass) is not a smarter accessor... we have name clashes and we cannot do anything.
- If all colliding methods are smart accessors, we could create one in the superclass, provided that there exists at least one subclass which does not provide its own implementation.

Of course there is not precise way of determining if a preexistent method can be considered as a smarter accessor or not... so we only can let the user decide.

To resume:
- Doing nothing will be the best default as it is suitable for all cases, and can not do any harm.
- Creating methods with alternative names could be optional, but shouldn't be default. In this case I prefer to ask the user for a name instead of creating one automatically.
- Creating the method even in case of collision can be dangerous... I am not sure if we should even allow it as an option.

2016-12-07 0:03 GMT+01:00 Ben Coman <[hidden email]>:
How about mostly keeping the existing behaviour, but default to
collisions being deselected?
Then at least you don't need to search for it.
It minimises the chance of missing it by mistake.
The deselected item easily stands out to be reselected if required.

P.S. An additional side idea: If collision is from a superclass then
append (Superclass>>name) to the list item, and clicking on it shows a
code dialog to the right similar to Spotter code review pane, to make
a review easier.

cheers -ben

On Tue, Dec 6, 2016 at 10:09 PM, Yuriy Tymchuk <[hidden email]> wrote:
> It shouldn’t be too intelligent. Keep the default behavior, and for each
> method add a 3-state checkbox: create (default), skip, force (override).
> Also having a usage recorder would be nice to know if developers are
> actually checking the other state more often than the default one.
>
> Uko
>
> On 6 Dec 2016, at 13:03, Thierry Goubier <[hidden email]> wrote:
>
>
>
> 2016-12-06 11:34 GMT+01:00 Denis Kudriashov <[hidden email]>:
>>
>>
>> 2016-12-06 11:28 GMT+01:00 Yuriy Tymchuk <[hidden email]>:
>>>
>>> Additionally it was annoying if a superclass has a method with the same
>>> name. For example if you have a name var and you create accessors I’d like
>>> to have actually a ’name’ getter and not ’name1’
>>
>>
>> Yes
>>
>
> If you start to make it so intelligent it requires half a dozen click to
> generate a simple accessor, then people will stop using the refactoring and
> write the code by hand...
>
> Thierry
>
>
>

Denis Kudriashov

Re: Generate accessors refactoring

In reply to this post by Eliot Miranda-2

Hi Eliot

2016-12-08 14:11 GMT+01:00 Eliot Miranda <[hidden email]>:

To be clear, you don't mean Do Nothing; you mean Issue an Error and Abort the Refactoring right?

No. "Do Nothing" means that if we already have method with instVar name we will skip this accessor. So we will not generate setter if there is already method #instVar:. No matter if this method do something wrong inside.

Denis Kudriashov

Re: Generate accessors refactoring

2016-12-08 14:18 GMT+01:00 Denis Kudriashov <[hidden email]>:

To be clear, you don't mean Do Nothing; you mean Issue an Error and Abort the Refactoring right?

No. "Do Nothing" means that if we already have method with instVar name we will skip this accessor. So we will not generate setter if there is already method #instVar:. No matter if this method do something wrong inside.

But getter will be generated in that case. No abort.

Nicolas Passerini

Re: Generate accessors refactoring

Yes, I meant what Denis says.

If you abort the refactoring/transformation in case of name clash, then you will have more cases in which the transformation is not applicable.

2016-12-08 14:19 GMT+01:00 Denis Kudriashov <[hidden email]>:

2016-12-08 14:18 GMT+01:00 Denis Kudriashov <[hidden email]>:
To be clear, you don't mean Do Nothing; you mean Issue an Error and Abort the Refactoring right?

No. "Do Nothing" means that if we already have method with instVar name we will skip this accessor. So we will not generate setter if there is already method #instVar:. No matter if this method do something wrong inside.

But getter will be generated in that case. No abort.