> On Dec 6, 2016, at 10:53 AM, Denis Kudriashov <[email protected]> wrote: > > > 2016-12-06 17:16 GMT+01:00 John Brant <[email protected]>: > > 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
