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

Whenever I come across it and I don't think changing it will cause problems (e.g. I can understand it enough that I know what it does. e.g. the level of voodoo is low).

Answer 2

Usually I don't refactor the code if I'm just browsing it, not actively working on it.

But sometimes ReSharper points out some stuff I just can't resist to Alt+Enter. :)

Answer 3

If the refactor makes the code much easier to read, the most common for me would be duplicate code, e.g. an if/else that only differs by the first/last commands.

if($something) {
  load_data($something);
} else {
  load_data($something);
  echo "Loaded";
  do_something_else();
}

Answer 4

I only bother to change it if there is some other reason I'm modifying the code.

How far I'm willing to take it depends on how confident I am that I won't break anything and how extensive my own changes to the code are going to be.

Answer 5

I tend to refactor very long functions and methods if I understand the set of inputs and outputs to a block.

This helps readability no end.

Answer 6

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.

Answer 7

I would only refactor the code that I come across and am not actively working on after going through the following steps:

• Speak with the author of the code (not always possible) to figure out what that piece of code does. Even if it is obvious to me as to what the piece of code is doing, it always helps to understand the rationale behind why the author may have decided to do something in a certain way. Spending a couple of minutes talking about it would not only help the original author understand your point of view, it also builds trust within the team.
• Know or find out what that piece of code is doing in order to test it after re-factoring (A build process with unit tests is very helpful here. It makes the whole thing quick and easy). Run the unit tests before and after the change and ensure nothing is breaking due to your changes.
• Send out a heads up to the team (if working with others) and let them know of the upcoming change so nobody is surprised when the change actually occurs

Answer 8

This is a great situation to show off the benefits of unit tests.

If unit tests are in place, developers can bravely and aggressively refactor oddly written code they might come across. If it passes the unit tests and you've increased readability, then you've done your good deed for the day and can move on.

Without unit tests, simplifying complex code that's filled with voodoo presents a great risk of breaking the code and not even knowing you've introduced a new bug! So most developers will take the cautious route and move on.

Answer 9

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 10

More than (arguably) three or four lines of duplicate code always makes me think about refactoring. Also, I tend to move code around a lot, extracting the code I predict to be used more frequently into a separate place - a class with its own well-defined purpose and responsibilites, or a static method of a static class (usually placed in my Utils.* namespace).

But, to answer your question, yes, there are lot of cases when making the code shorter does not necessarily mean making it well structued and readable. Using the ?? operator in C# is another example. What you also have to think about are the new features in your language of choice - e.g. LINQ can be used to do some stuff in a very elegant manner but also can make a very simple thing very unreadable and overly complex. You need to weigh these two thing very carefully, in the end it all boils down to your personal taste and, mostly, experience.

Well, this is another "it depends" answer, but I am afraid it has to be.

Answer 11

Refactoring for the sake of it is one of the roots of all evil. Please don't ever do it. (Unit tests do somewhat mitigate this).

Answer 12

This is a small, minor antipattern but it so irritates me that whenever I find it, I expunge it immediately. In C (or C++ or Java)

if (p)
    return true;
else
    return false;

becomes

return p;

In Scheme,

(if p #t #f)

becomes

p

and in ML

if p then true else false

becomes

p

I see this antipattern almost exclusively in code written by undergraduate students. I am definitely not making this up!!

Answer 13

I almost always break >2 similar conditionals into a switch... most often with regards to enums. I will short a return instead of a long statement.

ex:

if (condition) {
  //lots of code
  //returns value
} else {
  return null;
}

becomes:

if (!condition)
  return null;

//lots of code..
//return value

breaking out early reduces extra indents, and reduces long bits of code... also as a general rule I don't like methods with more than 10-15 lines of code. I like methods to have a singular purpose, even if creating more private methods internally.

Answer 14

Is vast swaths of commented out code an antipattern? While not code specifically, (it's comments) I see a lot of this kind of behaviour with people who don't understand how to use source control, and who want to keep the old code around for later so they can more easily go back if a bug was introduced. Whenever I see vast swaths of code that are commented out, I almost always remove them in their entirety.

Answer 15

I try to refactor based on following factors:

1. Do I understand enough to know whats happening?
2. Can I easily revert back if this change breaks the code.
3. Will I have enough time to revert the change back if it breaks the build.

And sometimes, if I have enough time, I refactor to learn. As in, I know my change may break the code, but I dont know where and how. So I change it anyways to find out where its breaking and why. That way I learn more about the code.

My domain has been mobile software (cell phones) where most of the code resides on my PC and wont impact others. Since I also maintain the CI build system for my company I can run a complete product build (for all phones) on the refactored code to ensure it doesnt break anything else. But thats my personal luxury which you may not have.

Answer 16

For simple refactoring I try to clean up deeply nested control structures and really long functions (more than one screen worth of text). However its not a great idea to refactor code without a good reason (especially in a big team of developers). In general, unless the refactoring will make a big improvement in the code or fix an egregious sin I try to leave well enough alone.

Not refactoring per-say but just as a matter of general housekeeping I generally do this stuff when I start work on a module:

• Remove stupid comments
  • Comments that say nothing more than the function signature already says
  • Comments that are pure idiocy like "just what it looks like"
  • Changelogs at the top of the file (we have version control for a reason)
  • Any API docs that are clearly out-of-sync with the code
• Remove commented-out chunks of code
• Add version control tags like $Id$ if they are missing
• Fix whitespace issues (this can be annoying to others though because your name shows up for a lot of lines in a diff even if all you did was change whitespace)
  • Remove whitespace at the end of the lines
  • Change tabs->spaces (for that is our convention where I work)

Answer 17

I tend to refactor global constants and enumerations quite a bit if they can be deemed a low refactor risk. Maybe it's just be, but something like a ClientAccountStatus enum should be in or close to the ClientAccount class rather than being in a GlobalConstAndEnum class.

Answer 18

Deletion/updating of comments which are clearly wrong or clearly pointless.
Removing them is:

1. safe
2. version control means you can find them again
3. improves the quality of the code to others and yourself

It is about the only 100% risk free refactoring I know.

Note that doing this in structured comments like javadoc comments is a different matter. Changing these is not risk free, as such they are very likely to be fixed/removed but not dead certain guaranteed fixes as standard incorrect code comments would be.