views:

176

answers:

4

Hello, I recently read Michael C. Feathers' book Working effectively with legacy code and came across the point where he mentioned a way to test the safety of automatic refactoring tools.

My question is: Are there any safe refactoring tools for the .net platform?; that means tools which only allow real refactorings and e.g. don't allow the inline variable refactoring on the temp variable in the following example, or at least show a warning that I am changing logic.

class Program
{
    private static int _x;

    static void Main()
    {
        int temp = Test();
        for (int i = 0; i < 10; ++i)
        {
            Console.WriteLine(temp);
        }
        Console.ReadKey();
    }


    private static int Test()
    {
        return ++_x;
    }
}

I've tested this example on the refactoring tools Resharper and Coderush + Refactor pro with the latest versions and both failed the test and allowed the refactoring to:

class Program
{
    private static int _x;

    static void Main()
    {
        for (int i = 0; i < 10; ++i)
        {
            Console.WriteLine(Test());
        }
        Console.ReadKey();
    }

    private static int Test()
    {
        return ++_x;
    }
}
+9  A: 

Refactoring is inherently risky. Relying solely on a tool to make your code safe is unwise imo.

We use Resharper but not without the safety net of comprehensive unit tests. I am not aware of any better C# tool in this space.

Steve Townsend
I agree on the point that you should always have unit tests to cover your refactoring, but actually if you had a tool which only allows safe refactorings, you could e.g. apply some basic refactorings before adding unit tests to legacy code or to make the code testable, which would make you more productive.
RoXX
Refactoring *shouldn't* be inherently risky from a functionality point of view. This just goes to show that the tools aren't strong enough to do the checking, so the tool builders offer them without the necessary checking. Buyer beware. (Don't get me wrong: it is *hard* to implement all the necessary checks: you need what amounts to the complete front end of a compiler).
Ira Baxter
+2  A: 

"Safe" is rather subjective....

While those two tools made not be considered "safe" based on this test in your mind both of those tools are extremely useful. No tool is perfect. If there is a situation where they do something you don't like either avoid doing it or create a work around.

klabranche
+5  A: 

I disagree that your "test" shows failure.

YOU changed the logic, not the tool. You changed the code such that a method would be called repeatedly instead of once.

The tools merely did what you told them to do.

Chris Marisic
Yup. This is why you should test your code after each refactoring so you *know* when you do change the logic.
strager
I would accept the OPs statement if the tool truly introduced the fault, such as using an invert if statement refactoring that resulted in a different logical outcome.
Chris Marisic
Actually I used the TOOL to apply a refactoring, which was not a refactoring. The definition of refactoring is changing code without modifying its behaviour, but the tool changed some behaviour so it shouldn't call this functionality refactoring. Of course having unit tests to cover this is always best, but actually you would be more productive if you could apply these automatic refactorings without having to worry about breaking existing functionality
RoXX
Chris Marisic
But actually what I did to the code through the tool was NOT a refactoring. The tool should only offer me the ability to do a refactoring, if it is really a refactoring, that means that it is not changing logic. So my opinion is that the tool should not offer the possibility to do something which is not a real refactoring, or atleast label it different / show some warning / whatever http://en.wikipedia.org/wiki/Code_refactoring
RoXX
@RoXX, Perhaps you can file a bug report or feature request for these "refactoring" tools to give you what you want. In the meantime, realize that automated refactorings *can* break code, just like compilers can.
strager
@strager, I will definitly add a feature request for my favorite tools, but there are many more productivity tools with refactorings for .net, like e.g. Visual Assist X and others, which I haven't tried out yet or maybe haven't heard of yet. Maybe one of the tools out there already has this functionality...
RoXX
@RoXX, what you want is pretty unrealistic IMHO. Also the refactoring name is quite correct, as it denominates a tool that you can use to do refactorings; of course you can use it for something else, just as you can use a brick to hammer a nail. It's not necessarily the best thing to do, but it works and is not the intended behavior of the brick, but there's no way to prevent you from doing it. Making a smart tool that would work in a majority of scenarios would be quite hard, especially with the immense freedom granted by programming languages nowadays.
Alex Paven
+2  A: 

To be really safe with automated refactorings is very hard.

When we first introduced refactorings in Visual C#, we asked ourselves this question: do our refactorings need to get things completely right all the time, or should we allow them to make mistakes in certain cases?

To be right all the time would require a lot of programmer effort, meaning we'd only get a few refactorings in the box. It would also make the refactorings slow, as they'd spend a lot of time in validation.

Allowing them to make mistakes would make them useless for any teams that didn't have great automated test coverage. TDD teams have good tests, but that's only one portion of the Visual Studio user base. We didn't want to make features that we had to tell people not to use!

TDD teams would catch mistakes quickly, but they would learn just as quickly not to trust our refactoring tools. They'd hesitate to use them, and seek other solutions much of the time (find-and-replace instead of Rename).

Also, as the C# team, we were in a good position to make high-fidelity refactorings. We had a unique advantage, with the C# language designers and compiler team just down the hall. We knew we should play to our strengths.

So, we decided to make fewer high-quality refactorings, instead of lots of refactorings that weren't as reliable. Today there are 6.

Visual Studio Refactorings

Looking back, I wish we had only done Rename, Extract Method, and Introduce Local Variable. Those last two are almost the same, implementation-wise. The 3 Parameter refactorings (there used to be a 7th, Promote Local Variable to Parameter, but it was cut in VS2010) were a ton of work to get right, and probably weren't worth it.

My recommendation is to do TDD, giving you a great collection of tests so you can refactor safely, whether you use a tool or do it by hand.

Jay Bazuzi