Refactoring Code - How Do You Know You Are Not Breaking It

UPDATE: The title of the post should have been "The Cost and Benefits of Emotion-Driven Code Improvements", but since this was an emotional and not well prepared post, I screwed it :) This is the third draft, and finally it looks understandable...

Last Saturday I felt a strange urge to go to the office and to finish what I had started the previous day. Basically I was frustrated by working with poorly formatted and commented code, that did not follow naming conventions and other good practices. I had also noticed a few flaws in some parts of the code, such as not guarding for exception between IDataReader's Open() and Close() methods (which was easily fixed by enclosing the reader within a using region) and other exception handling problems.

I thought that (in the long run) me and the whole team will be spending much more time and energy understanding such code, than the time and energy I wanted to spend to fix it. Yeah, most of the developer's time is spent reading the code. With the help of my beloved ReSharper, I reformatted all the code in one of the projects in the solution by fixing spacing, shortening references, removing unnecessary this qualifiers, removing unused code. Then I did some manual whitespace formatting and finally applied the functional fixes in the code. I did all those things in a single session intending to perform a single checking-in. And you better believe that later it turned out that it is impossible to quickly find what the functional changes were. No diff tool is that clever to show only the differences that can affect the behaviour of the program.

Lesson learned: Separate concerns, do one thing at a time, and checkin after it is completed.

I waited till Monday for the check-in, in order to get the approval of my PM, since this was not a planned operation. I was not surprised to hear that he was not very enthusiastic about such major code changes - every single file of a project with about 60 files was modified. He asked one of my colleagues to do a review of the changes. I had to explain that this is not as easy as it sounds and I offered an approach that would eliminate the non functional code changes altogether, by looking directly at the generated IL. Well, not the ugly IL itself, but better the output of its decompilation (of course, using Reflector).

I remembered that there is a Reflector addin, called Reflector Diff. It works very well, but only on per method basis, so it was virtually impossible to inspect all functional changes in a reasonable amount of time. We ended up using a statistical approach, examining just a couple of files and generalizing about the quality of the changes, heh :) Later I got some better ideas, but it was too late: (a) Decompile the old version and the new version of the assembly in separate folders with Reflector.FileDisassembler and diff them with a tool that supports folder diffs (like my favorite WinMerge). (b) Automatically reformat the old code with Resharper and then diff all the files in the folder.

Lesson learned: After making a mistake, there is often still a chance to find a good enough workaround. So know your tools and use them creatively.

After my code changes got approved, I tried to check-in them. Surprisingly it turned out that 3 of the files were already modified by other people and I was faced with the difficult task of merging them. I am not sure how this happened, I am pretty sure that when starting editing a non checked out file, VS.NET takes care to get its latest version before checking it out.

Unfortunately these were one of the files with the most functional changes in them. The automatic VSS merge naturally could not handle such major changes, so I had to perform it manually. What I did was to find out what the other people's changes were and apply them one by one. I had to be extra careful, so it took two more hours to get it done.

But there were more surprises in store. During the checkin process I was shown the default VSS merging screen to merge the changes. I knew that the current state of the files was already up-to-date so I chose to Apply all the changes from the right pane. Argh, hete it get very confusing, because it turns out that this operation does not copy the whole file to the merge result. It appears to do something different, so I ended up having a few unused methods in the result files, that were deleted in my version.

Lesson learned: Know in detail the behaviour of the tools you use, unless you like surprises :) When starting a major refactoring make sure that noone else is editing the code.

Aftermath: I think the mission was a success. Why?

• I can now work on a codebase that makes me feel comfortable and productive.
  
• I learned a few things along the way that could help do better the next time I have to clean bad looking code.
• I had a chance to show my colleagues the power of ReSharper, my standards for good code, and what was the state of our code base (btw, this was one of the best designed and documented parts in the solution). I guess that I should have done that a while ago. In fact I proposed such a course of action few months ago, but I don't think that I pointed clearly what the benefits were. But when people dwell in clean code for some time and see how good code feels, they realize that they don't have to work in the mess, so they are ready to make the necessary effort to do so.
• Finally, I was really amazed how productive and energetic I was, when working on something I thought would make a difference.