CSharp-Coding-Standards.md

#C# Coding Standards

###Refactoring

Embrace refactoring! Make code refactorable by adding tests using TDD. Really TDD exists to allow refactoring.

The term code smell comes from Martin Fowler's book Refactoring. If you haven't already, read this book. The examples are in Java but they easily translate to C#. Don't write new code that has smells by refactoring them out. Here is a list of the most egregious smells:

• Duplicated code.
• Long Method.
• Long Parameter List.
• Large Class.
• Conditional Complexity.
• Dead Code.
• Comments. See - Comments
• Too Many Dependencies
• Switch Statements.
• And More: http://c2.com/cgi/wiki?CodeSmell

##Class Design

###Stupid Constructor Keep class initialization simple. No logic should be in object constructors. Only assigment of constructor parameters to member fields/properties for the most part.

###Law of Demeter Follow the Law of Demeter (aka One . Law), classes/methods should only care about their immediate members. Not their members members (i.e. object.Member.MembersMember)

###SOLID

Great code can be described as SOLID.

http://en.wikipedia.org/wiki/SOLID_(object-oriented_design)

####Single Responsibility

Classes responsible for one thing. Anytime you say "and" when describing your class functionality the law is broken.

####Open/Closed

Classes are open to extension but closed for modification. This doesn't mean if the base class is wrong you shouldn't change it. It means that adding new features should not requiring altering the base class.

####Liskov Principal

Don't change the behavior in a subclass. Use an abstract class to share functionality. For example:

// Instead of this...

public class Base
{
    public virtual object Get()
    {
        return new { Name = "Base" };
    }
}

public class SubClass : Base
{
    public override object Get()
    {
        return "Sub with" + base.Get().Name;
    }
}

// ...which will make this break...

public string Get(Base param)
{
    return (param as dynamic).Name;
}

// ...do this.

public abstract class Base
{
    protected object BaseGet()
    {
        return new { Name = "Base" };
    }
    
    public abstract string Get();
}

public class SubClass : Base
{
    public override string Get()
    {
       return "Sub with" + Get().Name;
    }
}

The latter will result in more predictable behavior. Basically avoid the virtual keyword. See: C# Keyword Guide - Virtual

####Interface Segregation

Prefer more specific interfaces to generalized ones. For instance, an interface like...

public interface IAnimal
{
    void Sleep();
    void Fly();
}

...does seem convenient for cases where you may need Fly() behavior but because it isn't used by all types the result is more confusing and harder to maintain code. See Also: C# Keyword Guide - Interface

####Dependency Inversion

Depend on the abstraction and not the concrete implementation. This only becomes relevant when more than one class does a thing. For example:

// When you have this...

public interface IAnimal // Side Note: More than one class MUST 
{                        // implement this or remove it.
    void Sleep();
}

public class Dog : IAnimal
{
    public void Bark() { }
    public void Sleep() { }
}

// ...then this method...

public void MakeSleep(Dog dog)
{
    dog.Sleep(); // Notice only sleep is used!
}

// ...should be updated to this.

public void MakeSleep(IAnimal animal)
{
    animal.Sleep();
}

See Also: Patterns - Handling Dependencies

###Other Principals

####KISS, YAGNI and DRY

http://en.wikipedia.org/wiki/KISS_principle

http://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

http://en.wikipedia.org/wiki/Don%27t_repeat_yourself

TL;DR - Don't try to make code too fancy, don't add code you don't use now, and don't be a copy/paste machine.

##Conventions

###General

Use ReSharper and get your code to be ReSharper green.

###Comments

Only use comments to provide documentation as described here: http://msdn.microsoft.com/en-us/library/b2s063f7.aspx.

Rather than adding comments, refactor your method, class and variable names to adequately describe what code is doing and test names to describe why.

Don't check-in commented code. Remove the code.

Generally TODO comments hang around indefinitely so they are a red flag.

//TODO: create timebomb todo attribute as alternative to TODO comment.

###Naming

Take time to use a name that best describes the behavior of a method, class, variable, etc.

In absence of the perfect name default to the most descriptive name regardless of length. UpdateCustomerInfoToIncludeHistory is far better than Update or even UpdateCustomer.

Names should not be misleading. FindCustomer should not also update the customer name. A better name would be FindCustomerAndUpdateName. (Even better would be to make two methods FindCustomer and UpdateCustomerName)

Never use abbreviations in names.

Variables that represent a quantity or number should include units in their name. Especially for configuration values. int ExpirationTimeInSeconds rather than int ExpirationTime.

Don't include type in primitive variable names.

public int IntValue { get; } // Bad

public CustomException Exception { get; } // OK

When a name exists that is not ideal stick with that name unless all usages can be replaced. This includes database column names. For example:

// Don't change the name of the target property...

target.ClientId = source.BusinessCode

// ...unless you can change the property name of the source.

target.ClientId = source.ClientId

// When the target property means something new, make that clear.

target.BusinessCode = source.BusinessCode

public class Target
{
    public string ClientId { get { return BusinessCode; } }
    ...

###Method Results

A method that returns from execution without throwing an exception was successful and failures result in exceptions. Do not return a bool, enum or string representing success or failure unless that is the sole purpose of that method. The exception is when doing a TryParse as it has an established precedent.

###Exceptions

Always create custom exceptions, the more specific the better. Use existing custom exceptions if possible.

Never throw the following exceptions: Exception, ValidationException, ApplicationException.

throw Exception("cant find customr"); // BAD!

throw CustomerNotFoundException(); // BETTER

Custom exceptions should have the word Exception at the end. (SomethingBad should be SomethingBadException)

Avoid catching exceptions.

Do not use exception handling for control flow.

###Reflection and Clever Behavior

Avoid using reflection.

Avoid using recursion.

##Extension methods

Avoid creating extension methods. They are still static and therefore still pose testability concerns.

Note: it is sometimes acceptable to create extension methods on types that are not modifiable (e.g. third party or framework code).

###LINQ and IEnumerable

Listen to all the ReSharper warnings about multiple enumerations, access to a modified closure.

Avoid transformation during projection (Select). This is a missed opportunity for testing, and sometimes results in SQL errors.

// Don't do this.

orderItems.Select(x => x.Quantity == 0 ? "empty" : x.Quantity.ToString + " items");

LINQ is a quick way to exceed the aforementioned cyclomatic complexity guidelines. Remember to break complex lambdas into methods or rule classes when applicable.

Loops should be used if they result in more readable code. Ignore ReSharper if it suggests otherwise.

Avoid returning IQueryable.

##Patterns

###Separation of Concerns

Keep vertical concerns separate. The following are examples of vertical concerns: logging, exception handling, authentication, authorization, validation.

Do not write business logic inside of code that is generated from a template, or classes that are derived from framework code. For example, do not write business logic in an aspx.cs file, or within code that derives from Controller.

###Cyclomatic Complexity

Cyclomatic complexity can be estimated by imagining the number of unit tests required to fully cover every branch in a particular block of code. Ensure that any single class can be fully covered by no more than a handful of unit tests.

Complex conditional logic should be broken up into methods. If such methods would require more than a handful of unit tests then they should be broken into rule classes.

//TODO: Dave will add example.

###Handling Dependencies

Creating testable and maintainable code requires conscious effort in identifying and managing dependencies in your code. (e.g. IO, 3rd Party Services, Singletons, etc.)

###Gang of Four

####Value Type

Primitive obsession is something to avoid. Rather than passing a decimal you could create a value type to hold that primitive called say Money. This gives you many advantages such as:

1. Type-safety (i.e. Can't assign random decimal to Money)
2. Validation (e.g. Range throws exception when assigning invalid MinValue and MaxValue)
3. Readability, code intent is clearer.
4. Too easy to mess up Execute(int a,int b,int c,int d,string e,string f,string g);

####Singleton

Avoid this pattern. It makes your code untestable and therefore suck.

For example:

public string DoStuff();
{
  Database.TalkToDatabase(); // I'm a fancy singleton.
  return "Red";
}

[Test]
public void WhenDoStuffThenRed()
{
  Assert.AreEqual("Red", DoStuff()); // Can't get singleton out of way to just test result.
}

##C# Keyword Guide ###var

Use var whenever possible. Your code is still type-safe, it's less code and refactoring is easier. For example:

ISqlQuery query = context.ReturnSqlQuery();
return query.Execute();

// becomes

IObjectQuery query = context.ReturnObjectQuery();
return query.Execute();

// --- Versus ---

var query = context.ReturnSqlQuery();
return query.Execute();

// becomes

var query = context.ReturnObjectQuery();
return query.Execute();

###virtual

Avoid making things virtual. Instead use abstract or interface.

###catch

Never catch exceptions, use a global exception handler. Finally blocks acceptable if you need them.

###ref

Don't create methods using this.

###region

Don't use regions. Refactor code to make it easier to read. However, some generated code will use regions which do not need to be removed.

###out

Don't create methods using this. TryParse may be an exception due to precedent. For example:

int value;
if (int.TryParse("1234", out value)) {
    DoSomething(value);
}

###static

Do not use static fields or properties. Private static methods are the only acceptable static methods.

Public static methods are almost always the wrong decision.

Bad:

public static string IAmUpdatedAllOverThePlace;

Less Bad:

public static int Add(int first, int second)
{
    return first + second;
};

###new

The new keyword binds a concrete implementation to a code path making unit testing more difficult when used inside a method. There are three places where the new keyword is acceptable.

1. At the earliest code entry path possible.
2. In a poor man's dependency injection constructor. (Legacy code only)
3. In a factory method.

###interface

The best interfaces generalize a group of types. Here's an example of a nice interface:

public interface INamable
{
    string Name { get; }
}

Three interface rules:

1. Don't create interfaces that classes implement but are never used/useful.
2. Don't create interfaces only one class implements.
3. Keep interfaces named well and as small as possible. (i.e. remove unused parts)

##Testing

All code must have automated tests to support refactoring and continuous integration. Ideally these are unit tests but there will likely be some integration tests to test loading code.Bad automated tests will ruin productivity and lessen the overall test value.

The following are qualities of good automated tests:

• Organized. Arrange, Act, Assert or Given When Then
• DRY (careful here)
• Descriptive.
• Independent of other tests.
• Repeatable (Consistently passes, no random)
• Small in size, 15 lines max.
• One logic assert. (opt for more tests)
• Removed dependencies. (Db, services, etc.)
• Fast!

A change in production code should result in only one resulting compilation error in unit test code. More than this and there is likely some duplication.

Never break encapsulation to enable testing. In other words, don't subclass production code to get around access modifiers. Don't loosen the access modifiers just so the test code can call what should be a private method.

###Note on Mocking

Mocks are a great tool for prototyping or BDD (Behavior Driven Development) but should be viewed as stubbed code to be eventually removed. Mocks you distance your test code from reality and make refactoring more difficult because they know too much about how the system code looks. Try to separate the load, bind and execute portions of your code before relying on Mocks to solve a test dependency problem.

Moq is the preferred mocking framework.

Avoid writing hand-crafted mocks. If you find a need for hand-rolled mocks there is a good chance you are doing something wrong.

###Miško Testable Code For more information go here: http://misko.hevery.com/code-reviewers-guide/

Regarding the non-use of out, it is very handy in Try methods, e.g.

int value;
if (int.TryParse("1234", out value)) {
    DoSomething(value);
}

There are other ways to do this same thing, but all have flaws:

• returning a Tuple<bool, int> or a custom Maybe type
  • complex and awkward to get the result from, requiring more lines of code
• throwing an exception
  • in cases where we might expect the exception and handle the result differently, this is a terrible code smell.

Under C# Keywords add a comment to never use #region unless the code is generated by some other library / plugin / framework.