C# Code Smells. What are the most common and how to fix them?

Answer 1

Large classes with very long methods.

Fix: break up responsibilities to different classes, refactor to smaller methods. Do all this with good unit tests around the code.

Answer 2

I recommend you read Refactoring: Improving the Design of Existing Code by Martin Fowler, it give the code smell samples.

Answer 3

Circular depedency, but not always. And sometimes is really hard to get rid of that.

Answer 4

One that I see most of the time is the use of as instead of a cast, without any null checks. I don't know why people prefer to use as over a cast when they need a type check. I'd rather have an InvalidCastException than a NullReferenceException.

(control as TextBox).Text = ...;

Fix:

((TextBox) control).Text = ...;

Answer 5

Here's my list:

• Duplicate code
• Big functions
• Too many method arguments (>3)
• Using ref & out arguments
• God objects
• Deep inheritance structure
• Contrived complixity
• Refused bequest (when you break the LSP)
• Incorrect use of globals
• Empty catch clauses.
• Magic numbers.
• Multiple abstraction levels in one method.

Answer 6

Most general OO + imperative code smells will be applicable e.g. using explict branching instead of polymorhism, or very long methods

some more slightly more c# specific things things:

• leaking memory from event handlers
• overuse of reflection
• using non-generic containers to contain a single type

StyleCop (and FxCop) can be used to automate this of course

Answer 7

Mouldy comments

Where 3 year old commented-out test code stays in the code base:

// Not sure if this will be needed
// int result = command.Setup();
// Command.WriteLine(result)

Answer 8

A cousin of the large class and/or large functions: i hate it when one file contains many classes.

I have been guilty of this in the past, and it usually happens when you are coding something up in a hurry and you cannot be bothered creating logical files to house the classes, so you end up with one file containing something like 10 classes (i just refactored one of these today, and it wasn't one of mine).

A derivative of this is when one (usually badly named) file contains the definitions of 10 different enums. Usually enum definitions are not so long so it can be hard to justify putting them all in to different files, but if they are going to be communally contained within one file then that file should at least be named appropriately.

Answer 9

Pokemon exception handling

Coined here.

try
{

}
catch
{
   // I'm catching everything as I don't really 
   // know what my application is doing.
}

One 'exception' to this is where you're using 3rd party libraries, and the documentation is so thin you have no idea what exceptions the library might throw.

Answer 10

Use tools like Resharper. It gives suggestions and best practices to write code. It also examines coding conventions.

Answer 11

Not particular to C#, but are all valid anyway: http://wiki.java.net/bin/view/People/SmellsToRefactorings

Answer 12

Not using LINQ when applicable.

Or, in other words, implementing something that could have been implemented using LINQ in a smelly way. Codes like that can become unreadable easily. They also may conceal several responsibilities (hurting the Single Responsibility Principle) like filtering, sorting, selecting, grouping, etc. Even without LINQ, this type of code should be implemented in a clean way.

Eric Lippert has written about this: http://blogs.msdn.com/b/ericlippert/archive/2010/01/11/continuing-to-an-outer-loop.aspx

Answer 13

Having member or variable names that don't make their purpose clear, or having too many intermediate variables

This suggests disorganized planning/forethought, and likely also disorganized design. For instance a method implementation might have a load of intermediate variables that are not necessary, leading to names like "tempResultPartial1" and "pretransformAnswer2". I'm not saying intermediate variables are always wrong, just that too many might suggest the code is too complex.

Answer 14

Uncommented LINQ or lambda expressions. Lambda expressions without a sensible name for the variables.

Query type expressions often have unintuitive purposes, and are thus easily misunderstood by someone else reading the code. This is okay if it's a simple expression, but anything with a chain of calls will require a double take.

Also, if you're doing a lambda, you might as well call your variable something that tells the reader what it is. So for instance accounts.select(acnt=>acnt.acntID) makes more sense than accnts.select (x=>x.acntID). This becomes even more important when you chain calls together, for instance if you have a grouping.

Answer 15

This may be a personal thing for me, but I don't think "var" belongs in well-factored Object Oriented code (even though it is a "feature" of C#).

var thing = new Widget();
//...
// manipulate thing later in the code

Or even better, if I'm maintaining code and I see this:

foreach (var item in mysteriousCollection)
{
    item.MethodA();
    item.DoMoreStuff();
    // Manipulate item in other ways, you get the idea...
}

I saw that line of code, and I was like "what's an item? What methods and properties does it have?" Don't make me waste my time ferreting out what the heck the mysteriousCollection is, just so I can figure out what kind of thing the code is iterating through.

By some definitions, a code smell is a driver for refactoring. Consider the foreach loop example above. What's the easiest way to refactor out the code inside the loop into another method? private void ManipulateItem(var item) does not work (The contextual keyword 'var' may only appear within a local variable declaration). Now I've got to go figure out what the item is and how to deal with it.

From the comments, I can see some legitimate reasons why you would declare something "var" instead of the Parent class, but from a OO refactoring standpoint I think the use cases are limited. (Any other comments on good uses of var would still be appreciated)

Answer 16

Empty catch blocks like these....

 try{  
      //do something
    }
catch(SQLException sqex){  
        // do nothing  
    }

Exception handling for creating alternate method flows...

 try{  
     //do something  

 }catch(SQLException sqex){  

     //do something else  
 }

Answer 17

If my initials are somewhere in the .cs-file header or if my username is connected with the latest changeset, I know there is something wrong with the code that needs to be looked at. :-)

Answer 18

Complexity

Simply put, complexity is IMO the most common and most important code smell. Sure there are always bits here and there you can improve upon but if the overall complexity of your application/class/method/solution is too high you have a problem.

Complexity is often a sign that something hasn't been refactored enough, ie you added a bunch of stuff which created "scar tissue" and you didn't refactor to remove it. Sometimes when you add features you hit a "complexity barrier" where you need to introduce some new abstraction in order to keep each discrete part simple and easy to comprehend and digest. When looking at code, ask yourself:

• Does this method/class do more than one thing? Most often it shouldn't

• Does this method/class have a lot of dependencies to other methods/classes. They shouldn't

• If you hadn't written the code yourself and was looking at it, would it make sense?