Implement Interface (Redux)
Sorry for the poor appearance of some of the code samples. The new blog site seems to be having problems with the markup. I'm working with them to try and find a solution ASAP.
Ok. Until i find a solution, i'm just going to use screen captures.
We’ve made some recent changes to the Implement Interface (II) feature and I wanted to go over them here to let you know about them and to explain our reasoning for the changes. Hopefully you’ll agree with the choices we made and you’ll like the feature even more. I’ve always had some gripes with how II worked in the past (and in beta1 and the beta2 build you’re going to receive) and I decided that these gripes would actually be things that would be unpleasant for the user and really needed to be fixed. Specifically, the feature was not only confusing for the user in terms of the job it performed, but it was also pretty dumb when it came to doing that job.
(note: the same problems apply to Implement Abstract Class; but as that feature is basically identical to II I’m just going to refer to the entire issue by just dealing with II)
Let’s take a look at II back in beta1:
(Implement interface smart tag tickler)
(Implement interface with tickler expanded)
Ok, looks fairly good right? Underneath an interface that a type is declared to implement you get options on how to make this type implement that interface. So what’re the problems? Well, first off, the choices are just plain confusing for many people. Explicit interface implementation is not a feature that is often used (or even known) by many, and if you were to ask someone “what’s an implicit implementation?” they probably couldn’t tell you. But, that’s not really a huge problem. People will learn pretty quickly since they’ll probably try both options and will see how they differ. So what’s the real problem for me? Well, the fact that we’re basically lying to you when offering those options. What do I mean by that? Well consider the following code:
The problem, of course, is that it’s impossible to implement this interface implicitly. If you tried you would get:
which is illegal in C# (methods with the same parameters can’t have differing return types). Instead what you need to write is:
i.e. explicitly implement any methods which are going to conflict with other members (either from the same interface, or which already exist in the type). Note: Implement Abstract Class has a worse time since it has no way to explicitly implement members when there is a conflict. A later post will discuss this pesky issue).
So why did we call the option: “Implement interface ‘IFoo2’ stubs Implicitly”? Pretty much because in the beginning that’s all it did, and only when we ran into this issue did we realize that the option name was a rather poor one. So what should the option be called? We thought about: “Implement interface ‘IFoo2’ stubs Implicitly, unless there is a conflict, in which case implement the conflicting sub Explicitly”, but that seemed a bit too verbose and confusing, (and far too long for an onscreen option J), and so we settled on the following:
The first option now means: “implement the interface doing whatever necessary to make legal code”, i.e. implicitly when possible, but explicitly if not; whereas the second is for people who really want everything to be explicit.
Ok, so this is a tiny bit better than before. But I’m sure you’re saying: “you know, I really couldn’t care less about a minor wording change like that”, and really, if that was all we changed I’d agree that that wasn’t worth talking about. However, since there was a whole bunch of work done here, I wanted to talk about all of it. So now onto change #2:
Let’s look at another example:
Now, prior to my changes what you’d actually end up with was:
i.e. besides just implementing the members in IList, we’d also reimplement the ones in ICollection. The logic was set up so that when you implemented an interface we would implement all of its methods, regardless of whether they’d already been implemented by a supertype. As it turns out this is completely legal in C# and, of course, it made the implementation of the II feature quite simple. However, whenever I used the feature this ended up being so aggravating since it was never what I wanted. CyrusList already has an Add method (inherited from CyrusCollection) so why re-implement it, especially since all the user is going to do is hunt through the newly spit code and just delete these extraneous methods.
We struggled over this for a while. Were there users who would prefer our model of spitting out everything? After all, if you don’t like it you can always remove the extra junk spit in the code, but if you do want it then there is no way to get it. But, in the end we decided that in the common case our default behavior would just end up annoying the user and so we went back to our definition of what “Implement Interface” meant and revised our definition to:
“implement the interface doing the minimal amount of work necessary to make legal code”
We feel that when a user selects this first option what they are really saying is: “look, there is some grunt work that needs to be done so that I actually implement this interface and get into a compilable state. Just do that work for me so I can get on with the actual interesting work of putting the right logic into those new members”. And so doing the minimal work now fits in with providing them a helpful service without tripping them up.
There’s also one more thing we changed about the II feature. Let’s go back to the original example.
You decide to implement the interface, and sweet, the interface is implemented for you!
Except… this is odd, there’s still a smart tag under the interface declaration. What happens if i expand it?
Hunh… “implement interface implicitly” is still in the list. Waitaminute… I just implemented that interface! Why is that option still there? Let me try using it… and it does nothing! Sweet! Offering an option that silently does nothing when invoked!! That’s always a popular feature in any application, and really is something that good designers tell you you should be doing.
Turns out there’s already a feature we have in VS2005 that has behavior that’s more in line with what we’d like to see here: Generate Method Stub (GMS). If we see that there’s already an existing method that you could be calling, then we don’t offer to generate the stub for you. This is, of course, completely necessary. If we didn’t do this then under every method call you’d see this silly smart tag and you’d probably kill us for distracting you with all that clutter. However, just because there we be less clutter with II (since there are less interfaces implemented than method calls in your code) doesn’t mean that it’s ok to keep offering you useless options. So our final change to II was to make it so that if it has no work to do then it doesn’t both even offering the option to you. The experience is now a lot nicer for you and fits the GMS model where we offer the option and then don’t show it after you’ve used it.
Now, this was somewhat of a large functionality change to make late in the game, and so I wanted to do it in the simplest way possible (i.e. DoTheSimplestThingThatCouldPossiblyWork). So what I did was look at the logic that we currently have for II and I broke it into two parts. The first part does all the work walking interface lists and determining which members need to be implemented and it also generates all the code that needs to be spit back into the user’s code. i.e. it’s all the heavy lifting. All the second part does is literally call ReplaceLines on the user’s buffer with the code generated by the first part. After refactoring the code in that way, all I did was make it so that when we query to determine if the II option is available we do the entire first step. Then, if we see that that produced no text to spit into the code then we respond by saying “no, this option is not available”. This refactoring allowed me to make an extremely small code change that didn’t actually affect any of the core logic of II (which I was worried about touching and possibly screwing up), and left us with the very desirable model of: “if we know we would generate nothing then don’t even bother offering the option.” Some people see that as being a fugly change. “Why go through all that work, especially generating the code, if you’re not even going to use it? What a waste when you could be doing this so much more efficiently.” But, because of making this code change in this way, we were able to make the feature change with high confidence that it would not be a problem and we would not be regressing any behavior very late in the game.
I hope you like the changes that we’ve made here. Unfortunately you won’t see them until past beta2, but since I think that you’re going to be using this feature a lot I thought it would be worth it to spend that little extra effort on it to improve the entire user experience around it.
Let me know what you think!