Generate accessors refactoring

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

Generate accessors refactoring

Denis Kudriashov
Hi.

There is very annoying logic of accessors refactoring. 
For example imaging your class already has getter for variable #myInstVar but with some extra code instead of simple return. For example it could be lazy initialization:

MyClass>>myInstVar
   ^myInstVar ifNil: [#someValue]

In that case accessors generator will suggest you new getter method #myInstVar1. 
I really doubt that anybody accept generation of such method. 
I always remove it from changes list which is very annoying.

Am I alone about it? Can we remove this logic?

Best regards,
Denis
Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Aliaksei Syrel
+1
super annoying!

On Dec 6, 2016 11:10, "Denis Kudriashov" <[hidden email]> wrote:
Hi.

There is very annoying logic of accessors refactoring. 
For example imaging your class already has getter for variable #myInstVar but with some extra code instead of simple return. For example it could be lazy initialization:

MyClass>>myInstVar
   ^myInstVar ifNil: [#someValue]

In that case accessors generator will suggest you new getter method #myInstVar1. 
I really doubt that anybody accept generation of such method. 
I always remove it from changes list which is very annoying.

Am I alone about it? Can we remove this logic?

Best regards,
Denis
Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Nicolai Hess-3-2
In reply to this post by Denis Kudriashov


2016-12-06 11:09 GMT+01:00 Denis Kudriashov <[hidden email]>:
Hi.

There is very annoying logic of accessors refactoring. 
For example imaging your class already has getter for variable #myInstVar but with some extra code instead of simple return. For example it could be lazy initialization:

MyClass>>myInstVar
   ^myInstVar ifNil: [#someValue]

In that case accessors generator will suggest you new getter method #myInstVar1. 
I really doubt that anybody accept generation of such method. 
I always remove it from changes list which is very annoying.

Am I alone about it? Can we remove this logic?

Best regards,
Denis

18880 Autogenerating accessors should be less naive
Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Uko2
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’

Uko

On 6 Dec 2016, at 11:16, Nicolai Hess <[hidden email]> wrote:



2016-12-06 11:09 GMT+01:00 Denis Kudriashov <[hidden email]>:
Hi.

There is very annoying logic of accessors refactoring. 
For example imaging your class already has getter for variable #myInstVar but with some extra code instead of simple return. For example it could be lazy initialization:

MyClass>>myInstVar
   ^myInstVar ifNil: [#someValue]

In that case accessors generator will suggest you new getter method #myInstVar1. 
I really doubt that anybody accept generation of such method. 
I always remove it from changes list which is very annoying.

Am I alone about it? Can we remove this logic?

Best regards,
Denis

18880 Autogenerating accessors should be less naive

Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Denis Kudriashov

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

Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Thierry Goubier


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
 

Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Uko2
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
 


Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Denis Kudriashov

2016-12-06 15:09 GMT+01:00 Yuriy Tymchuk <[hidden email]>:
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.

For me it is too intelligent now because it is tried to create new version of accessors instead of existing one. 
But simple logic should just analyze method name and if accessor already exists it should skip it
Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

John Brant-2

> On Dec 6, 2016, at 8:25 AM, Denis Kudriashov <[hidden email]> wrote:
>
>
> 2016-12-06 15:09 GMT+01:00 Yuriy Tymchuk <[hidden email]>:
> 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.
>
> For me it is too intelligent now because it is tried to create new version of accessors instead of existing one.
> But simple logic should just analyze method name and if accessor already exists it should skip it

That isn’t a refactoring. The existing methods aren’t accessors. The refactoring will find accessors for the variables if they exist (even if they have a different names).

New accessors can’t override an existing method, for example #name, since that would change the behavior of the #name method for that object. If you are suggesting not creating the accessor method for the “myInstVar ^myInstVar ifNil: [#someValue]” method, then you’ll need to change the abstract variable refactorings so that they no longer use the modified create accessors refactoring. Otherwise you can change behavior. For example, code such as “myInstVar ifNil: [#someOtherValue]” would no longer be able to return #someOtherValue and would instead return #someValue.

My personal preference would be to request some additional input from the user in these cases. I don’t know if the 3-state checkbox or something else would be best. Anyway, we chose adding a number to the end of the name so that these methods would be easy to rename since they would have a low likelihood of conflicting with already existing method names. For example, no one implements #name1 in my image.

BTW, it appears that someone changed the refactoring errors to be information messages that are displayed at the bottom (and quickly go away and sometimes appear behind your browser or other windows). This allows someone to perform a refactoring that failed its preconditions. If you are not looking at the bottom of the screen when the message appears you think everything is fine when it isn’t. For example, it let me perform the abstract class variable refactoring on DependentFields in Object (which breaks your image). Also, since the refactorings were written so that an error stopped the refactoring from running, you can now get debuggers when some future step of the refactoring is being executed when it should be executed. For example, try extracting some code with a return that isn’t the last part of a method:

        foo
                self someTest ifTrue: [ ^true ].
                ^false

If you try to extract the first line, you get an information notice at the bottom that you can’t extract the code since it has a return, but the extract method keeps running, and you get a debugger when the refactoring is being performed on something that failed the preconditions.


John Brant
Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Denis Kudriashov

2016-12-06 17:16 GMT+01:00 John Brant <[hidden email]>:
> For me it is too intelligent now because it is tried to create new version of accessors instead of existing one.
> But simple logic should just analyze method name and if accessor already exists it should skip it

That isn’t a refactoring. The existing methods aren’t accessors. The refactoring will find accessors for the variables if they exist (even if they have a different names).

We all know what is refactoring. Usually we don't care about safe aspect of it (and we have tests). We just want simple code transformation. I think it is exactly such case. 
Also after applying override browser shows it with special icon. And user could see it.
 

New accessors can’t override an existing method, for example #name, since that would change the behavior of the #name method for that object. If you are suggesting not creating the accessor method for the “myInstVar ^myInstVar ifNil: [#someValue]” method, then you’ll need to change the abstract variable refactorings so that they no longer use the modified create accessors refactoring.

We could make it configurable. "Manually" accessors will be generated in simplified mode and related refactorings will use intelligent mode
Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

stepharong

> For me it is too intelligent now because it is tried to create new version of accessors instead of existing one.
> But simple logic should just analyze method name and if accessor already exists it should skip it

That isn’t a refactoring. The existing methods aren’t accessors. The refactoring will find accessors for the variables if they exist (even if they have a different names).

We all know what is refactoring. Usually we don't care about safe aspect of it (and we have tests). We just want simple code transformation. I think it is exactly such case. 

+ 1

Often I get a popup telling me something that I do not understand and I ended doing all the changes.
So if the situation is 
    refactoring or doit by hand
    I prefer automatic transformations under my responsibilities because else I have to do it by hand. 

I like the idea of the radio buttons to control the behavior. 

Also after applying override browser shows it with special icon. And user could see it.
 

New accessors can’t override an existing method, for example #name, since that would change the behavior of the #name method for that object. If you are suggesting not creating the accessor method for the “myInstVar ^myInstVar ifNil: [#someValue]” method, then you’ll need to change the abstract variable refactorings so that they no longer use the modified create accessors refactoring.

We could make it configurable. "Manually" accessors will be generated in simplified mode and related refactorings will use intelligent mode



--
Using Opera's mail client: http://www.opera.com/mail/
Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Nicolai Hess-3-2
In reply to this post by John Brant-2


2016-12-06 17:16 GMT+01:00 John Brant <[hidden email]>:

> On Dec 6, 2016, at 8:25 AM, Denis Kudriashov <[hidden email]> wrote:
>
>
> 2016-12-06 15:09 GMT+01:00 Yuriy Tymchuk <[hidden email]>:
> 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.
>
> For me it is too intelligent now because it is tried to create new version of accessors instead of existing one.
> But simple logic should just analyze method name and if accessor already exists it should skip it

That isn’t a refactoring. The existing methods aren’t accessors. The refactoring will find accessors for the variables if they exist (even if they have a different names).

New accessors can’t override an existing method, for example #name, since that would change the behavior of the #name method for that object. If you are suggesting not creating the accessor method for the “myInstVar ^myInstVar ifNil: [#someValue]” method, then you’ll need to change the abstract variable refactorings so that they no longer use the modified create accessors refactoring. Otherwise you can change behavior. For example, code such as “myInstVar ifNil: [#someOtherValue]” would no longer be able to return #someOtherValue and would instead return #someValue.

My personal preference would be to request some additional input from the user in these cases. I don’t know if the 3-state checkbox or something else would be best. Anyway, we chose adding a number to the end of the name so that these methods would be easy to rename since they would have a low likelihood of conflicting with already existing method names. For example, no one implements #name1 in my image.

BTW, it appears that someone changed the refactoring errors to be information messages that are displayed at the bottom (and quickly go away and sometimes appear behind your browser or other windows).

Yes, this is really bad. I tried to fix this but it is not taht easy. (And what is even worse, in the meantime some refactoring precondition were changed to work with this behavior (Ok, I am not totally innocent, some of that
changes are from me, because it took some time to understand how the refactoring preconditions and refactoring errors are working)) But we should change this to a real error handling.
 
This allows someone to perform a refactoring that failed its preconditions. If you are not looking at the bottom of the screen when the message appears you think everything is fine when it isn’t. For example, it let me perform the abstract class variable refactoring on DependentFields in Object (which breaks your image). Also, since the refactorings were written so that an error stopped the refactoring from running, you can now get debuggers when some future step of the refactoring is being executed when it should be executed. For example, try extracting some code with a return that isn’t the last part of a method:

        foo
                self someTest ifTrue: [ ^true ].
                ^false

If you try to extract the first line, you get an information notice at the bottom that you can’t extract the code since it has a return, but the extract method keeps running, and you get a debugger when the refactoring is being performed on something that failed the preconditions.


John Brant

Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Nicolai Hess-3-2
In reply to this post by Denis Kudriashov


2016-12-06 17:53 GMT+01:00 Denis Kudriashov <[hidden email]>:

2016-12-06 17:16 GMT+01:00 John Brant <[hidden email]>:
> For me it is too intelligent now because it is tried to create new version of accessors instead of existing one.
> But simple logic should just analyze method name and if accessor already exists it should skip it

That isn’t a refactoring. The existing methods aren’t accessors. The refactoring will find accessors for the variables if they exist (even if they have a different names).

We all know what is refactoring. Usually we don't care about safe aspect of it (and we have tests). We just want simple code transformation. I think it is exactly such case. 
Also after applying override browser shows it with special icon. And user could see it.
 

New accessors can’t override an existing method, for example #name, since that would change the behavior of the #name method for that object. If you are suggesting not creating the accessor method for the “myInstVar ^myInstVar ifNil: [#someValue]” method, then you’ll need to change the abstract variable refactorings so that they no longer use the modified create accessors refactoring.

We could make it configurable. "Manually" accessors will be generated in simplified mode and related refactorings will use intelligent mode

The whole refactoring engine is pretty well written, we should take care to not fix the wrong places (this has been done already in the past by people (including me) not understanding the framework well enough).
 

Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

John Brant-2
In reply to this post by Denis Kudriashov
> On Dec 6, 2016, at 10:53 AM, Denis Kudriashov <[hidden email]> wrote:
>
>
> 2016-12-06 17:16 GMT+01:00 John Brant <[hidden email]>:
> > For me it is too intelligent now because it is tried to create new version of accessors instead of existing one.
> > But simple logic should just analyze method name and if accessor already exists it should skip it
>
> That isn’t a refactoring. The existing methods aren’t accessors. The refactoring will find accessors for the variables if they exist (even if they have a different names).
>
> We all know what is refactoring. Usually we don't care about safe aspect of it (and we have tests). We just want simple code transformation. I think it is exactly such case.

“We all” probably don’t agree on much. In fact, I think many people would argue that the “safe aspect” is a very important part of a refactoring. Otherwise, they shouldn’t be called refactorings, but should instead be called transformations. If you call something a “refactoring” but don’t do basic checks on its validity then that is just wrong.

> Also after applying override browser shows it with special icon. And user could see it.

Assuming that you didn’t override something that is an essential part of the image. For example, try overriding the class side method #name to return nil and tell me how the special icon works.

>
> New accessors can’t override an existing method, for example #name, since that would change the behavior of the #name method for that object. If you are suggesting not creating the accessor method for the “myInstVar ^myInstVar ifNil: [#someValue]” method, then you’ll need to change the abstract variable refactorings so that they no longer use the modified create accessors refactoring.
>
> We could make it configurable. "Manually" accessors will be generated in simplified mode and related refactorings will use intelligent mode

My personal preference would be to ask the user what to do when performing the accessor refactoring. If the class hierarchy already has a method with the name of the variable, we can look at that method and determine what to do. In the case of #name, we could see that it is defined in a superclass and ask if it is ok to override the method (likely a good choice, but not always so it is good to confirm with the user). If the class directly defines the method, and that method has a return that returns the variable, then we can ask if they want to use that method as the “accessor” even though it does some other stuff (and potential returns a different value). Finally, if some subclass defines the method, then we can ask if they want to create that method even though some subclass is overriding it. Of course, we could have any combination of all three too. If they want a different method, then we should ask what to name the method instead of appending a number to the name.

Currently, the extract method works similarly. If you enter an existing hierarchy method name, it warns you that you could be changing the behavior and asks if that is what you want to do. By asking the user, we can confirm that is what they really want to do, and also let them know of the potential problems that they may not have been aware of.


John Brant
Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

stepharong

>> That isn’t a refactoring. The existing methods aren’t accessors. The  
>> refactoring will find accessors for the variables if they exist (even  
>> if they have a different names).
>>
>> We all know what is refactoring. Usually we don't care about safe  
>> aspect of it (and we have tests). We just want simple code  
>> transformation. I think it is exactly such case.
>
> “We all” probably don’t agree on much. In fact, I think many people  
> would argue that the “safe aspect” is a very important part of a  
> refactoring. Otherwise, they shouldn’t be called refactorings, but  
> should instead be called transformations. If you call something a  
> “refactoring” but don’t do basic checks on its validity then that is  
> just wrong.

Yes this is why we are working on proposing refactorings and  
transformations.
We are currently refactoring the refactorings to have both transformations  
and refactorings.

Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Ben Coman
In reply to this post by Uko2
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
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Denis Kudriashov
In reply to this post by John Brant-2

2016-12-06 19:29 GMT+01:00 John Brant <[hidden email]>:
> We could make it configurable. "Manually" accessors will be generated in simplified mode and related refactorings will use intelligent mode

My personal preference would be to ask the user what to do when performing the accessor refactoring. If the class hierarchy already has a method with the name of the variable, we can look at that method and determine what to do. In the case of #name, we could see that it is defined in a superclass and ask if it is ok to override the method (likely a good choice, but not always so it is good to confirm with the user). If the class directly defines the method, and that method has a return that returns the variable, then we can ask if they want to use that method as the “accessor” even though it does some other stuff (and potential returns a different value). Finally, if some subclass defines the method, then we can ask if they want to create that method even though some subclass is overriding it. Of course, we could have any combination of all three too. If they want a different method, then we should ask what to name the method instead of appending a number to the name.

Ok. You prefer tons of questions to user just to perform stupid accessors generation. And, please, don't say that it is rare cases. It is quite common which nicely shown here 18880.
I wonder do you have real experience when it protected you from destroying system? We only talk about simple accessors refactoring.
Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Nicolai Hess-3-2


2016-12-07 10:44 GMT+01:00 Denis Kudriashov <[hidden email]>:

2016-12-06 19:29 GMT+01:00 John Brant <[hidden email]>:
> We could make it configurable. "Manually" accessors will be generated in simplified mode and related refactorings will use intelligent mode

My personal preference would be to ask the user what to do when performing the accessor refactoring. If the class hierarchy already has a method with the name of the variable, we can look at that method and determine what to do. In the case of #name, we could see that it is defined in a superclass and ask if it is ok to override the method (likely a good choice, but not always so it is good to confirm with the user). If the class directly defines the method, and that method has a return that returns the variable, then we can ask if they want to use that method as the “accessor” even though it does some other stuff (and potential returns a different value). Finally, if some subclass defines the method, then we can ask if they want to create that method even though some subclass is overriding it. Of course, we could have any combination of all three too. If they want a different method, then we should ask what to name the method instead of appending a number to the name.

Ok. You prefer tons of questions to user just to perform stupid accessors generation. And, please, don't say that it is rare cases. It is quite common which nicely shown here 18880.
I wonder do you have real experience when it protected you from destroying system? We only talk about simple accessors refactoring.


Possible solution in inbox for case 18880. Needs a test case. As far as I can see,  this change does not conflict with application of abstract-variable-refactoring, but needs some testing (exising RB-Tests are green).

Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

John Brant-2
In reply to this post by Denis Kudriashov
> On Dec 7, 2016, at 3:44 AM, Denis Kudriashov <[hidden email]> wrote:
>
>
> 2016-12-06 19:29 GMT+01:00 John Brant <[hidden email]>:
> > We could make it configurable. "Manually" accessors will be generated in simplified mode and related refactorings will use intelligent mode
>
> My personal preference would be to ask the user what to do when performing the accessor refactoring. If the class hierarchy already has a method with the name of the variable, we can look at that method and determine what to do. In the case of #name, we could see that it is defined in a superclass and ask if it is ok to override the method (likely a good choice, but not always so it is good to confirm with the user). If the class directly defines the method, and that method has a return that returns the variable, then we can ask if they want to use that method as the “accessor” even though it does some other stuff (and potential returns a different value). Finally, if some subclass defines the method, then we can ask if they want to create that method even though some subclass is overriding it. Of course, we could have any combination of all three too. If they want a different method, then we should ask what to name the method instead of appending a number to the name.
>
> Ok. You prefer tons of questions to user just to perform stupid accessors generation. And, please, don't say that it is rare cases. It is quite common which nicely shown here 18880.
> I wonder do you have real experience when it protected you from destroying system? We only talk about simple accessors refactoring.

Yes, I would prefer a question to doing something wrong and breaking my code. Part of the problem appears to be what is defined as an "accessor”. Accessors as defined for refactoring are simple get/set accessors (the set version can return or not return the set value — depending on usage). Using this definition of accessor we can prove things about the code. However, when you extend the “accessor” definition then it gets much tougher to prove anything.

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

        foo: anInteger
                (anInteger between: 1 and: 100) ifTrue: [foo := anInteger]

        foo: anInteger
                (anInteger between: 1 and: 100)
                        ifTrue: [foo := anInteger]
                        ifFalse: [self error: ‘Invalid value’]

        foo: anInteger
                (anInteger between: 1 and: 100)
                        ifTrue: [foo := anInteger]
                        ifFalse: [bar := anInteger]

        foo: anInteger
                foo := anInteger.
                self changed

        foo: anInteger
                self aboutToChange: #foo.
                foo := anInteger.
                self changed: #foo

        foo: anInteger
                self change: #foo to: anInteger

        foo: anInteger
                “do nothing”

        foo: anInteger
                bar := anInteger

        bar: anInteger
                foo := anInteger

As for whether things are “rare” or not, that would depend on the usage and the person performing the refactoring. If you are in the habit of creating some accessor like methods and then doing a create accessors refactoring, then you will get this behavior frequently. However, if you are defining the class and then immediately creating the accessors, then it will be much less frequent.

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, #+ 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. We could have had the rename method refactoring handle this case also, but that would have required some non-portable code.


John Brant
Reply | Threaded
Open this post in threaded view
|

Re: Generate accessors refactoring

Eliot Miranda-2


On Wed, Dec 7, 2016 at 9:00 AM, John Brant <[hidden email]> wrote:
> On Dec 7, 2016, at 3:44 AM, Denis Kudriashov <[hidden email]> wrote:
>
>
> 2016-12-06 19:29 GMT+01:00 John Brant <[hidden email]>:
> > We could make it configurable. "Manually" accessors will be generated in simplified mode and related refactorings will use intelligent mode
>
> My personal preference would be to ask the user what to do when performing the accessor refactoring. If the class hierarchy already has a method with the name of the variable, we can look at that method and determine what to do. In the case of #name, we could see that it is defined in a superclass and ask if it is ok to override the method (likely a good choice, but not always so it is good to confirm with the user). If the class directly defines the method, and that method has a return that returns the variable, then we can ask if they want to use that method as the “accessor” even though it does some other stuff (and potential returns a different value). Finally, if some subclass defines the method, then we can ask if they want to create that method even though some subclass is overriding it. Of course, we could have any combination of all three too. If they want a different method, then we should ask what to name the method instead of appending a number to the name.
>
> Ok. You prefer tons of questions to user just to perform stupid accessors generation. And, please, don't say that it is rare cases. It is quite common which nicely shown here 18880.
> I wonder do you have real experience when it protected you from destroying system? We only talk about simple accessors refactoring.

Yes, I would prefer a question to doing something wrong and breaking my code. Part of the problem appears to be what is defined as an "accessor”. Accessors as defined for refactoring are simple get/set accessors (the set version can return or not return the set value — depending on usage). Using this definition of accessor we can prove things about the code. However, when you extend the “accessor” definition then it gets much tougher to prove anything.

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

        foo: anInteger
                (anInteger between: 1 and: 100) ifTrue: [foo := anInteger]

        foo: anInteger
                (anInteger between: 1 and: 100)
                        ifTrue: [foo := anInteger]
                        ifFalse: [self error: ‘Invalid value’]

        foo: anInteger
                (anInteger between: 1 and: 100)
                        ifTrue: [foo := anInteger]
                        ifFalse: [bar := anInteger]

        foo: anInteger
                foo := anInteger.
                self changed

        foo: anInteger
                self aboutToChange: #foo.
                foo := anInteger.
                self changed: #foo

        foo: anInteger
                self change: #foo to: anInteger

        foo: anInteger
                “do nothing”

        foo: anInteger
                bar := anInteger

        bar: anInteger
                foo := anInteger

As for whether things are “rare” or not, that would depend on the usage and the person performing the refactoring. If you are in the habit of creating some accessor like methods and then doing a create accessors refactoring, then you will get this behavior frequently. However, if you are defining the class and then immediately creating the accessors, then it will be much less frequent.

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, #+ 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. We could have had the rename method refactoring handle this case also, but that would have required some non-portable code.


John Brant

Amen.  If it ain't broke, don't fix it.  It should go without saying that if it ain't broke, don't break it.  Changing the accessor refactoring is breaking things.

_,,,^..^,,,_
best, Eliot
12