Refactoring Code to Avoid Anti-Pattern

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

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.

How to prevent the arrowhead anti-pattern

I'd go for the multiple return statements. This makes the code easy to read and understand.

Don't use goto for obvious reasons.

Don't use exceptions because the check you are doing isn't exceptional, it's something you can expect so you should just take that into account. Programming against exceptions is also an anti-pattern.

Eliminating `switch` statements

Switch-statements are not an antipattern per se, but if you're coding object oriented you should consider if the use of a switch is better solved with polymorphism instead of using a switch statement.

With polymorphism, this:

foreach (var animal in zoo) {
    switch (typeof(animal)) {
        case "dog":
            echo animal.bark();
            break;

        case "cat":
            echo animal.meow();
            break;
    }
}

becomes this:

foreach (var animal in zoo) {
    echo animal.speak();
}

How to overcome the anti-pattern Big Ball of Mud?

A Big Ball Of Mud normally occurs because of one of the following:

Change of Requirements - You architect a solution with one set of requirements, which over time change and now, you are probably catering to a different audience who wants to use the same product with slightly different requirements. You bake those requirements into the same product and you end up with a BBOM.
Change of Developers - The original product has been created by one set of developers with certain design and architectural assumptions which are not entirely evident to a whole new set of developers who 'take over' the product for maintainence or further development. The new developers make their own assumptions and over time, the product degrades into a pile of unmaintainable junk.
Incompetency - of the developers (they are unaware of anti-patterns), the management (too demanding, lack of knowledge of the product) or the users (they don't really know what they need). This is hard to solve.

Sometimes, the best solution is simply to rewrite the application catering to the new requirements. But this is normally the worst case scenario. The cumbersome solution is to stop all new development, start by writing a set of tests and then redesign and rearchitect the whole solution. This could take years, depending on the size of the product, though.

Premature refactoring?

I actually think the opposite.

The earlier you start thinking about whether or not your design needs refactoring, the better. Refactor constantly, so it's never a large issue.

I've also found that the more I refactor early on, the better I've gotten about writing code more cleanly up front. I tend to create fewer large methods, and have fewer problems.

However, if you find yourself "refactoring" yourself into a corner, I'd expect that is more a matter of lack of initial design or lack of planning for the scope of use of a class. Try writing out how you want to use the class or framework before you start writing the code - it may help you avoid that issue. This is also I think one advantage to test driven design - it helps you force yourself to look at using your object before it's written.

Remember, refactoring technically should NEVER lock you into a corner - it's about reworking the internals without changing how a class is used. If your trapping yourself by refactoring, it means your initial design was flawed.

Chances are you'll find that, over time, this issue gets better and better. Your class and framework design will probably end up more flexible.

When do I need to stop using design patterns?

If the code is working, and doesn't need attention - don't spend time/money updating it. Not until it is fiscally-necessary to do so. Just make sure all of your new code is excellent, and slowly erase this issue from now on.

Design patterns to avoid

Patterns are complex

All design patterns should be used with care. In my opinion you should refactor towards patterns when there is a valid reason to do so instead of implementing a pattern right away. The general problem with using patterns is that they add complexity. Overuse of patterns makes a given application or system cumbersome to further develop and maintain.

Most of the time, there is a simple solution, and you won't need to apply any specific pattern. A good rule of thumb is to use a pattern whenever pieces of code tend to be replaced or need to be changed often and be prepared to take on the caveat of complex code when using a pattern.

Remember that your goal should be simplicity and employ a pattern if you see a practical need to support change in your code.

Principles over patterns

It may seem like a moot to use patterns if they can evidently lead to over-engineered and complex solutions. However it is instead much more interesting for a programmer to read up on design techniques and principles that lay the foundation for most of the patterns. In fact one of my favorite books on 'design patterns' stresses this by reiterating on what principles are applicable on the pattern in question. They are simple enough to be useful than patterns in terms of relevance. Some of the principles are general enough to encompass more than object oriented programming (OOP), such as Liskov Substitution Principle, as long as you can build modules of your code.

There are a multitude of design principles but those described in the first chapter of GoF book are quite useful to start with.

Program to an 'interface', not an 'implementation'. (Gang of Four 1995:18)
Favor 'object composition' over 'class inheritance'. (Gang of Four 1995:20)

Let those sink in on you for a while. It should be noted that when GoF was written an interface means anything that is an abstraction (which also means super classes), not to be confused with the interface as a type in Java or C#. The second principle comes from the observed overuse of inheritance which is sadly still common today.

From there you can read up on SOLID principles which was made known by Robert Cecil Martin (aka. Uncle Bob). Scott Hanselman interviewed Uncle Bob in a podcast about these principles:

Single Responsibility Principle
Open Closed Principle
Liskov Substitution Principle
Interface Segregation Principle
Dependency Inversion Principle

These principles are a good start to read up on and discuss with your peers. You may find that the principles interweave with each other and with other processes such as separation of concerns and dependency injection. After doing TDD for a while you also may find that these principles come naturally in practice as you need to follow them to some degree in order to create isolated and repeatable unit tests.

When to refactor to a design-pattern?

If the code is readable/understandable and isn't likely to be changed or extended in the future, you may want to leave it the way it is.

But if the code will change, or you need to build a workaround for it at some point, you're better off refactoring it right away. And it's a good practice to check-in code cleaner than it was before.

Keep in mind though, it depends on the return on investment. Simple refactoring may cascade into other parts of the code, that then have to be refactored as well. If the code base won't be under constant development in the future, it may not be worth spending too much time refactoring it.