Almost 4 months after the first post in the series… not bad!
There are 3 Signature Change Refactorings:
I rank these as “Tier 2” – they’re not as important as Extract Method and Rename, but they’re still pretty hard to do by hand. They also have some complexity that can’t be resolved easily, making them slightly less reliable than the Tier 1 Refactorings.
The first attempt
Our first attempt created one feature called Signature Change. There was a single menu item & a single piece of UI. You were to invoke them on a method in your code, showing a form you could interact with. There you would party on the signature for a while, then commit all your changes.
This approach is very Microsoft – build a full-featured tool that doesn’t just enable a scenario, but covers every contingency we can anticipate. Think “one stop shopping”. The form had a lot of buttons & stuff. It was huge.
It was also wrong.
It didn’t match the Elvis attribute of being “code focused”. That is, we think that the C# customer is more comfortable manipulating code in an editor than in a form. (Contrast with Mort, who prefers forms to code.) This is the same reason we removed things like “Add Method” in the class view.
Once you were in this UI, it was only natural to expect it do even more, like rename the method or a parameter. This continued to add to the complexity of the UI & hurt usability.
It also didn’t match the way people really manipulate method signatures. You don’t look at a method and say “this method has 5 things wrong with its signature, and now I’m going to fix them”. You’re more likely to say “this method needs a new parameter for the work I plan to do” or “this method works correctly, but doesn’t match my coding convention of having all ‘out’ params at the end of the argument list”. That is, sig. change Refactorings are always small single steps and the UI we built was optimized for big steps (really multiple small steps).
As you can see, all this added up to it being the wrong UI, at least for Elvis (the typical C# customer). It was a very typical Microsoft way of building a feature, so it took us a little time to get on the right track.
As a special case of the “code focused” bit, it really worked poorly for “Add Parameter”. You click on the button and get a small dialog with 3 fields (name, type, default value). You then get to a fully qualified type name into an edit field. That’s no fun, especially since typing in the editor gets you a familiar, well-tuned IntelliSense experience. Finally, the name “default value” was confusing, because it seemed to suggest C# language support for default parameter values like those that exist in C++ and VB.
The second try
So, we took the UI and tore it apart. In the editor you can now click on a method (at the call site or definition) and request a Remove Parameters or Reorder Parameters.
A form appears allowing you to move parameters up & down on a list. Once they’re to your liking, you can apply the change.
This isn’t quite the ideal, code-focused model I would have liked. You’re acting on the whole method’s parameter list, but I think that acting on single parameters might have been better. Instead of being able to arbitrarily reorder all the parameters on a method at once, suppose we just supplied “move right” and “move left” operations on each parameter individually? Suppose they were extremely fast. That would probably be a simpler, more streamlined experience, and be more Code Focused.
Similar UI to Reorder, you can “delete” one or more parameters from the list.
Similar to my concerns about Reorder, I think that a model focused on removing a single parameter would be a better experience than this model, which allows you to remove multiple parameters at once.
Remove carries a small risk of changing the behavior of your code. Consider:
Removing the parameter will change the behavior of your code. We considered building more smarts protect against this, and then decided not to solve the halting problem for Whidbey. Instead, we put a warning on the UI that this Refactoring could change the behavior of your code. You can use the “Preview Changes” option if you’re worried. (Personally I avoid coding styles that allow this problem, by differentiating between “query” and “action” methods, but we still need to make sure that we’re building tools for our customers, not for ourselves.)
Similar issues are possible in the Reorder case, but even less common, so we decided not to clutter the UI with a warning in that case.
For both Reorder and Remove, you can invoke them via the context menu or the top-level Refactor menu, at either a call site or the definition. This brings up a modal form to manipulate the selected method.
You can also invoke these 2 Refactorings using “smart tags”. We put one on the definition of each method in your code, as a way of indicating “there are special things you can do here”. For both Refactorings, the smart tag offers Reorder and Remove actions.
Aside: Users seem mildly annoyed by smart tags in Office usability studies. If they had a more positive reaction there, you would have seem more aggressive use of smart tags in VS. For example, we could place a smart tag on every definition, of fields, locals, parameters, types, methods, etc. Most would offer at least rename, and each might offer other cool stuff. For example, the smart tag on a parameter might also offer the “Move Left” and “Move Right” actions I mentioned previously.
Smart tags are a lot like context menus, but even more focused on the location & should provide greatly added value, compared to the context menu & other sources of actions.
They seem to solve a certain class of discoverability & clutter issues. For example, suppose we decided to put “Move Left” & “Move Right” on the context menu. If we only show them in the menu when you click on a parameter, then users will see it once without realizing what the context was, and then get frustrated trying to find it again. If we always show it on the context menu, no matter where you click, then the context menu quickly becomes cluttered.
These are the things we spend our days fretting over!
Where’s “Add Parameter” in all this? Well, it’s not in Whidbey. I was convinced that there wasn’t a good user model for providing this Refactoring, at least with the facilities we had available at the time.
The problem is that no one wants to type type names into dialogs. It’s just no fun. Completion lists are where it’s at. (We talked about showing a completion list on the dialog, but morphing the code to support that seemed pretty expensive. I’m also not convinced that it would be an ideal experience.)
There was also this really odd bit where you had to supply a default value to pass in at all existing call sites. I already mentioned that the name “default value” was confusing because that name already refers to something in C++ and VB. It also caused problems because typing in the default value in an edit box isn’t ideal, either, like the type name. If you’re going to pass anything other than a simple constant, you really want to have IntelliSense helping you out. (Imagine passing “new SomeType(param1, param2)” – you’d want parameter help on that constructor, right?)
Really, the notion of typing this stuff in to a form just doesn’t fit with the code-focused nature of the typical C# customer.
We decided to just cut the feature for Whidbey, and come back to it later.
Then we came up with a cheap alternative that seemed to add a lot of value. Suppose you have a method with this line of code in it:
bool b = true;
Notice that it has exactly the right data for “Add Parameters” – name, type, value. We could build a Refactoring that would take an initialized local variable & “promote” it to a parameter. The type & name of the new parameter are exactly that of the local variable. The value to pass in at all the call sites is the value on the left of the equals. You got to write the type & the value using your favorite tool for the job (the C# editor, of course!).
What I’m talking about doesn’t solve all “Add Parameter” needs in the way we would have liked. It wasn’t clear that users who were looking for a way to add a parameter would think of trying this, and we wondered if users would just be plain confused by the whole idea. We also looked at the schedule and saw that there wasn’t much time to fit something like this in, especially if it wasn’t going to be the “ideal” solution.
We decided to take the cheapest approach possible. If people don’t like the feature, it wouldn’t be a big loss. It would fit in without causing the schedule to slip. So we first applied a set of constraints:
Looking back, I’m really glad things worked out this way. It’s very XP-like. We took the smallest step that delivered the greatest value to our customer. If we did this more of the time, we would ship a lot more often!
As XP-ers know, building working software generates a lot of learning. As soon as we started exploring Promote Local, we saw that it needed a bit more flexibility to really be useful, so we changed the constraints somewhat:
This allows some really important cases, like “Foo f = new Foo(“hi”);”. Note that we don’t fix up any type names on the right side. We just take the text in your code, as typed, and copy it to all call sites. It’s the simplest approach I could think of!
In the normal Microsoft way of thinking, we might have decided to build up a lot of logic to decide if the thing on the right was acceptable for the call sites. We could analyze visibility of any methods called, and offer to change their accessibility. We could offer to Encapsulate Field any fields that might be needed at the call site. We could fully qualify type names on the right, or offer to add ‘using’ directives as needed. But that would have been too expensive to deliver in Whidbey (we would have had to cut something clearly more important), so instead we would have cut Promote Local. But, applying more of the thinking we’ve learned from XP, we decided to continue to deliver this simple, cheap Promote Local.
I’ve heard people ask that we be smart about placing the parameter in the correct location (before all ‘out’, ‘ref’, and ‘params’ parameters), or offer UI that lets the user select the position of the new parameter. I argue that you’d rather accept the default of first position, and then later apply Reorder Parameters if you want to change things. This lets you take a smaller step & get back to delivering customer value sooner. (To be fair, there are many who disagree with me on this point.)
Promote Local is the only Refactoring we built that doesn’t have a form to take input. That means there isn’t a good place to put an option to Preview Changes. I get requests that we do something about this, but I really like the streamlined behavior we have today. Click on a local variable, invoke the command, and you’re done. That’s it. (Obviously, there are many who disagree with me on this point, too.)
Aside: Earlier I mentioned what we might have done with more smart tags. If we put a smart tag on all local variable definitions, one of the items on it would be “Promote to Parameter”.
Two other things about Promote Local are important to me:
Missing from the picture are the “ret <-> out” Refactorings.
The first takes a method with no return type (“void”) and changes one of its ‘out’ parameters to be the return type.
void F(out int x) //...
int F() //...
x = F();
Seems pretty cool, but there was a lot of complexity to overcome.
The second is the inverse (takes a return value and makes it an “out” parameter). This one is even harder to do, because sometimes the return value is the input to another method:
G(F()); // how do we do ret->out on F?
These would have been nice to have when you use Extract Method; it removes the question of how to decide what to do with outputs from the new method (“all ‘out’ params? Select one to be return and others be ‘out’?”).
However, the value of these Refactorings is lower than a lot of other very cool stuff, and the costs were pretty high due to the complexity involved. So they didn’t make it in Whidbey.
In the case of all of these Signature Change Refactorings, it’s clear we could have done a bit more work to make them even cooler. But as I mentioned in the beginning, these are “Tier 2”, so we would rather spend less time on them in favor of our Tier 1 Refactorings.
So, what do you think? Did we get it right? Will these Refactorings, as written, suit your needs?
Should we have stuck with the single form for all? Or continued pushing in the small steps direction? If we provided both models, which would you use?
Is Promote Local interesting and valuable as-is, or just a waste of time & UI space?
Do you need the ret<->out params more than we realized? Would you rather we cut something else in their favor?
For Beta 2, should we punt a set of bugs to make time to enhance these Refactorings?
If you want to discuss, comment here. If you have stronger feedback, then send it via Ladybug.