What types of coding anti-patterns do you always refactor when you cross them?

Question

I just refactored some code that was in a different section of the class I was working on because it was a series of nested conditional operators (?:) that was made a ton clearer by a fairly simple switch statement (C#).

When will you touch code that isn't directly what you are working on to make it more clear?

Answer 1

I once was refactoring and came across something like this code:

string strMyString;
try
{
  strMyString = Session["MySessionVar"].ToString();
}
catch
{
  strMyString = "";
}

Resharper pointed out that the .ToString() was redundant, so I took it out. Unfortunately, that ended up breaking the code. Whenever MySessionVar was null, it wasn't causing the NullReferenceException that the code relied on to bump it down to the catch block. I know, this was some sad code. But I did learn a good lesson from it. Don't rapidly go through old code relying on a tool to help you do the refactoring - think it through yourself.

I did end up refactoring it as follows:

string strMyString = Session["MySessionVar"] ?? "";

Update: Since this post is being upvoted and technically doesn't contain an answer to the question, I figured I should actually answer the question. (Ok, it was bothering me to the point that I was actually dreaming about it.)

Personally I ask myself a few questions before refactoring.

1) Is the system under source control? If so, go ahead and refactor because you can always roll back if something breaks.

2) Do unit tests exist for the functionality I am altering? If so, great! Refactor. The danger here is that the existence of unit tests don't indicate the accuracy and scope of said unit tests. Good unit tests should pick up any breaking changes.

3) Do I thoroughly understand the code I am refactoring? If there's no source control and no tests and I don't really understand the code I am changing, that's a red flag. I'd need to get more comfortable with the code before refactoring.

In case #3 I would probably spend the time to actually track all of the code that is currently using the method I am refactoring. Depending on the scope of the code this could be easy or impossible (ie. if it's a public API). If it comes down to being a public API then you really need to understand the original intent of the code from a business perspective.

Answer 2

I only refactor it if tests are already in place. If not, it's usually not worth my time to write tests for and refactor presumably working code.