Hi Nicolas, > On Dec 8, 2016, at 1:16 AM, Nicolas Passerini <[email protected]> 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 <[email protected]>: >> 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 <[email protected]> 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 <[email protected]> wrote: >> > >> > >> > >> > 2016-12-06 11:34 GMT+01:00 Denis Kudriashov <[email protected]>: >> >> >> >> >> >> 2016-12-06 11:28 GMT+01:00 Yuriy Tymchuk <[email protected]>: >> >>> >> >>> 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 >> > >> > >> > >> >
