An interesting issue came up today that many people on the C# team had a passionate discussion about. It was about the following code:

class C<T> {
    void M(T t) {
         t.ToString();
         Console.WriteLine(t.ToString());
    }
}
Where the user was extracting the first "t.ToString()" line. In that case we currently produce the following:
class C<T> {
    void M(T t) {
         NewMethod(t);
         Console.WriteLine(t.ToString());
    }
    void NewMethod(T t) {
         t.ToString();
    }
}
What's wrong with that? It seems completely reasonable. But consider the following code:
struct WeirdStruct {
     int i;
     public override string ToString() {
          i++;
          return i.ToString();
     }
}

...

C<WeirdStruct> c = new C<WeirdStruct>();
c.M(new WeirdStruct());

Before the refactoring we will produce the output "2". however after the refactoring we'll produce "1". Why? Because of a fun property of structs and how they are passed by value. After the refactoring we will actually pass a copy of the struct to the new method and the call to ToString will affect that copy only.

Because we don't know if the type variable will be instantiated with reference type or a value we don't know if it will ever be safe to just pass the type variable. The only way to make it safe would be to produce the following code:

    void M(T t) {
         NewMethod(ref t);
         Console.WriteLine(t.ToString());
    }
    void NewMethod(ref T t) {
         t.ToString();
    }

Notice that we now pass the type variables around as ref parameters. (In the reference case this won't affect anything, and in the struct case it will solve the above problem. However, some of us were worried about what this would be like for users using 'extract method'. They'd extract code that used type variables and would get pretty confused. Chances are that they would have written the code that we currently produce and if we generated the new code they wouldn't understand why and they'd remove the 'ref' from the parameters.

Several solutions were discussed, namely:

  1. Keep the current implementation, potentially producing code that might not produce the same results as the original
  2. Make all type passed as ref parameters, potentially producing confusion and dissuading people from using the feature
  3. Offer both behaviors through an option on the message box that we pop up to ask the user for the new method name

I bring this up because it goes deep to the heart of what you think a refactoring is. Is the purpose to help you modify your code safe in the knowledge that the meaning will stay the same? Is the purpose to help you make a large change in your code in the way you yourself would have done it, but to do it much faster than it would take you? Should you have enough unit tests so that issues like this would be caught and it's ok to possibly change the meaning of the code? Would you rather have to fix up our refactorings manually after we performed them, or would you prefer to try to track down a bug that was caused by the refactoring? Does it depend on the refactoring. i.e. are you willing to have some refactorings behave one way and others behave the other way? Is there a point at which you pick between one or the other or is it always clear cut for you ? What do you think the best solution is? I'd love to hear your thoughts on this.