mostlylucid

scott galloway's personal blog...
posts - 916, comments - 758, trackbacks - 11

My Links

News

Archives

Post Categories

Misc. Coding

Best Practices for .NET developers

This is something I'm doing for my team at work...thought you may like a look / comment etc...

Know the SOLID principles...

See http://butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod (the first 5). These are fundamental principles for good OO software design, know them...love them!

If you don't need it...delete it!

As for Code Comments below, never check in an Excluded Class to the source control system. It adds noise and is a great way to have hard to trace exceptions if they're accidentally re-included.

Code comments

I know it's in the policy document but to be honest no-one uses generated docs when tests are a better alternative for learning code.

Code comments should only be used to clarify complex algorithms, apart from that they just add cruft to code and make it harder to read. Method / parameter names should be descriptive (I frankly don't care how long they are, that's what intellisense is for!). If you can't work out what a method does from its name...rename it. Or if a method does more stuff than you can reasonably fit in a name, you need to refactor the method.

Keep classes compact

No hard and fast rule here but if you're >200 lines in a single class think seriously whether you need to refactor. If you follow rule 1 of SOLID (Single Responsibility Principle) then this should rarely happen.

Keep names meaningful

There are, no prizes for being terse...the more readable your code, the easier it is for you and others to find bugs.

Use 'var' instead of the type name where appropriate.

In general it's both neater and increases code readability to replace an often lengthy type name with the 'var' keyword. The main counterpoint to this is when the return type of an operation is not obvious from the name of the method / context.

Instead of:

StringBuilder sb = new StringBuilder(256);
UTF8Encoding e = new UTF8Encoding();
MD5CryptoServiceProvider md5 = new MD5CryptoServiceProvider();

We'd end up with this

var sb = new StringBuilder(256);
var e = new UTF8Encoding();
var md5 = new MD5CryptoServiceProvider();

Use the 'as' operator when casting reference types

The 'as' operator avoids runtime exceptions (e.g., (string)foo["bar"] could well throw an exception), in addition it is far more performant that doing an 'is' followed by an explcit cast.

Prefer enums over bools when they are used as return types from methods.

Enumerators make your code more readable (e.g. returning the value Car.Red from is CarColor rather returning true from the method IsCarRed) , they also make it far easier to extend code later on (true and false being rather limiting...)

Exceptions should only be used to indicate Technical Errors

Exceptions in .NET are valuable and when used well are a great feature. However, they should never be used as a tool in normal programmatic flow, only when an object does not perform an operation according to its specification / interface should an exception be returned.

Never catch an exception you can't do something with

Unless you actually do something with an exception, let it bubble up to somewhere which can do something (even if just reporting it to the user)

Use nullable types sparingly

Nullable types encourage optional parameters...optional parameters encourage branching within methods and reduce testability. Avoid them.

Never call the GC manually

It's smarter than you (oh, EXCEPT if you're using unmanaged objects)

Always use generic collection types over their non-generic ancestors

The generic collections are faster, avoid boxing and are basically cooler

The same applies to the new Concurrent collections when multithreading in 4.

Use the TPL

In .NET 4 when using concurrency prefer the new TPL http://msdn.microsoft.com/en-us/library/dd460717.aspx functions over managing threads 'by hand'

Prefer simplicity over more complex language features

Keeping your code simple to follow is critical to making changes easy and relatively low risk, one of the simplest ways to achieve this is to keep your code as transparent and simple to follow as you can.

Write, Test, Refactor, Test

Well factored code is easier to read, easier to change and easier to debug...A great test for well factored code is the 'Single Responsibility Principal', each method should really only perform a single operation on an object (for example, a single method should never both update and delete, or compress and save to disk).

· Avoid branching within methods
· Where you can avoid recursive methods

Avoid Regions

If you can't navigate through a single class file without collapsing sections of the code then your class is almost certainly in need of refactoring.

Regions add noise and make it harder to read code.

Only place they can be valuable is with long test classes...

Avoid Singletons

They mostly avoid concurrency issues at the expense of performance...there's almost always a better way.

Use Conditional Statements and Conditional Directives

When used properly, Conditional Debug Statements and conditional directives such as those below can be extremely useful in adding extra information to help you debug apps:

#if DEBUG
...
#endif

and

[Conditional("DEBUG")]

The main advantage of this method of adding additional debug information is that they are not output when the code is compiled in release mode.

If you DO use nullable types NEVER cast directly to the non nullable form

Again, this is a potential source of runtime exceptions. Instead, always use the .GetValueOrDefault() method and test for the default value (if necessary) before trying to use the value.

Avoid using the 'this.' Keyword

If your naming scheme is correct, you should never have to use 'this.' to specify that an object is local to your class.

Print | posted on Tuesday, January 18, 2011 5:46 AM | Filed Under [ .NET ]

Feedback

Gravatar

# re: Best Practices for .NET developers

Regarding using enums instead of bools, I'd argue that's not true for C# 4. Using the named parameter at the callsite with bool actually ends up being more readable.h

1/18/2011 7:19 PM | Haacked

Gravatar

# re: Best Practices for .NET developers

"The 'as' operator avoids runtime exceptions" - no, it turns an InvalidCastException right now into a NullReferenceException later on in the program. If I've casted to the wrong type I want my code to blow up with an obvious exception, not struggle along and fail later.

"I frankly don't care how long [method and class names] are" - if you kept your method and class names succinct then you wouldn't need to recommend using 'var' in variable declarations.

1/19/2011 12:52 AM | Tim Robinson

Gravatar

# re: Best Practices for .NET developers

Thanks Scott for this nice post.

1/19/2011 1:01 AM | Ashraful Alam

Gravatar

# re: Best Practices for .NET developers

Tim beat me to it :-) I also don't agree with using 'as' instead of explicit casting as a generalisation. I would use it when trying to cast to a type and invoking functions on something of that type if there is a chance of it not being the expected type. Even then it feels wrong when using OO. If you have a bunch of if statements checking types before calling functions you should be using polymorphism instead.

I'm a much happier dev seeing an InvalidCastException at the line of code that's broken rather than a NullReferenceException where the code isn't broken :-)

Cheers!

1/19/2011 1:45 AM | OJ

Gravatar

# re: Best Practices for .NET developers

THe pattern for 'as' is as follows:

public string Toad
{
get
{
var toadOut = MyCollection["toad"] as string;
return toadOut ?? string.Empty;
}
}

Yopu obviously ALWAYS need to check for empty.

Generics obviously provides a colution to the example above but when there's even the slightest chance of an Invalidcast you need to code defensively.

Oh, and as for the var stuff...nothing to do with long type names. It DOES make code more readable...

1/19/2011 3:54 AM | scott

Gravatar

# re: Best Practices for .NET developers

Don't call new MD5CryptoServiceProvider() directly instead use the MD5.Create() factory. This lets you and the framework configure swapping out algorithm implementations such as for an MD5 implementation in hardware.

1/19/2011 4:04 AM | John Downey

Gravatar

# re: Best Practices for .NET developers

That pattern for 'as' is worrisome in my opinion: what if MyCollection["toad"] is actually (and unintentionally) of type Stream?

As for the var stuff: code readability is always subjective... Personally I tend to find code more readable with explicit type names, but it's a matter of taste.

1/19/2011 4:17 AM | Dan Puzey

Gravatar

# re: Best Practices for .NET developers

Regarding your "var", instead of:

instead of:
var sb = new StringBuilder(256);
var e = new UTF8Encoding();
var md5 = new MD5CryptoServiceProvider();

we could end up with:
var htmlElements = new StringBuilder(256);
var textEncoder = new UTF8Encoding();

...

i.e. instead of relying on the type so much, try and put emphasis on naming methods and variables better. Then you won't need types to make code readable.

1/19/2011 5:05 AM | Hadi Hariri

Gravatar

# re: Best Practices for .NET developers

Nice summary,
thanks

1/19/2011 5:55 AM | Andrey

Gravatar

# re: Best Practices for .NET developers

@haacked...not really sure what you mean there.

1/19/2011 6:17 AM | scott

Gravatar

# re: Best Practices for .NET developers

Hadi...yeah totally..need to change that sample (was copied from elsewhere...the R# blog I believe :))
Scott

1/19/2011 6:18 AM | scott

Gravatar

# re: Best Practices for .NET developers

John, I'd argue that's just a good practice 'use a factory where a type may change' could even be use DI nowadays.

1/19/2011 6:22 AM | scott

Gravatar

# re: Best Practices for .NET developers

I would say using 'as' in place of direct cast is an anti-pattern. There are places for it, but it is certainly not a replacement for a direct cast. As others have pointed out, all using 'as' does is obfuscate the real cause of the error later. Consider this example:

var textBox = sender as TextBox;

textBox.Text = "Hi!"; //Assume this throws a NullReferenceException.

But *why* did it throw a NullReferenceException? You can no longer tell. It could be because 'sender' was not castable to a TextBox, or it could be because sender was actually null.

1/19/2011 9:51 AM | Matt Honeycutt

Gravatar

# re: Best Practices for .NET developers

W.R.T. "as" ... Scott is NOT advocating no check for null afterwards. Obviously there would be.

W.R.T. using "var" ... long/short method names have very little relevance here. The main use-case is when you're newing up an object, as all of Scott's examples imply. You also do not often have control of the class names you're using. Who wants to name the type/class of the object you're creating twice when you don't need to????

1/19/2011 1:42 PM | Chris Rogers

Gravatar

# re: Best Practices for .NET developers

@scott:

Haacked (hi Phil!) is arguing that something like this:

var res = Foo(isred: true);

is better than perhaps:

var res = Foo(Color.Red);

The problems I would potentially have with Phil's argument are (warning; rambling ahead)

- named parameters can apply to enum params as well, so if you think they make bool input params more readable, the same would seem to apply to any other type as well. I'll admit that *without* named params, bools are often ridiculous to try and understand. I've run across methods with 4 consecutive bool params (before C# 4) and as you'd imagine, trying to understand WTF was going on was a PITA. Now imagine you maintain that API and wish the ordering were different or whatever. You likely end up going to an Options param with 4 get; set; bool properties, which in effect (IMHO) just forces the named parameters :)

- even with named params, it takes more code (and more reading/parsing effort, IMHO). Passing 'Color.Red' vs. 'colorIsRed: true'. The latter (at least to me) takes longer to parse/grok as I'm reading code (especially code I'm not familiar with).

- the 'extensibility' argument (already mentioned). API's I was designing had to change from bool's to enum's often because something picked up a third option (visibility, recursion, deleted state, operation mode, etc)

- (true for all named arguments) you now have a source-code dependency on the name of the variable. if it's currently 'bool recursive' and I want to update it to 'bool isRecursive', I have to worry about places using it as a named param (outside of my immediate solution / set of projects). Admittedly this is pretty weak, but you're adding a dependency between the caller and callee (param name) that is there only because you're trying to make up for your weak param type :)

- bool isn't domain-specific. An API should surface and consume things that are domain-specific whenever possible. WPF/Silverlight lets me pass Visibility.Collapsed (admittedly WPF has 3 values for it), Regex lets me pass RegexOptions, String.Split lets me pass StringSplitOptions - that last one is a great example of a BCL choice towards the enum, as it only has 2 values (None, RemoveEmptyEntries) so it very well could have been a bool. :)

- for API creation, the killer to me is that you can't force callers to use named params. I make an API in a common object model with a bool param then ask everyone else in my company (or project, or whoever is consuming it) to always use a named param for clarity. Not very realistic, IMHO. Even if you could, you're asking the API consumer to expend more effort and also asking all code reviewers to remember that rule (or enforcing with StyleCop, or whatever).

1/19/2011 7:58 PM | James Manning

Gravatar

# re: Best Practices for .NET developers

@scott:

could you make this 'Enumerations' instead of 'Enumerators'? The current phrasing is IMHO potentially confusing for those that might be used to IEnumerable/IEnumerator. Also, AFAIK 'Enumeration' is the 'standard' name for them, at least checking msdn.microsoft.com/en-us/library/cc138362.aspx

***
Enumerators make your code more readable
***

1/19/2011 8:01 PM | James Manning

Gravatar

# re: Best Practices for .NET developers

Very interesting comments...I liked OJ"s comment about seeing an InvalidCastException in the right place instead of a NullReferenceException in the wrong place. I do have to agree about using the AS operator but mostly focused on the scenario that scott shows, where you can do a very single one time check instead of risking a NullReferenceException further down the line, like OJ.

James Manning....agree. I was corrected during a technical discussion once...it's a good thing to use the right term, Enumeration.

1/24/2011 10:22 AM | Gabriel Rodriguez

Gravatar

# re: Best Practices for .NET developers

Ah, i almost forgot.

One advantage of using the var keyword instead of the actual type name, is that if you're using an IoC container or a Factory to create your objects, you wouldn't need to modify your class at all if you change types later.

You could use an interface...but still. I think var is easier and does provide more readability.

1/24/2011 10:24 AM | Gabriel Rodriguez

Gravatar

# re: Best Practices for .NET developers

Nice post, Scott! 2 Comments from my side.

I agree 100% with your view on code comments. The obvious and glaring exception is when you're writing a library - it's extremely frustrating to use a library where the author simply didn't bother with comments.

On the 'as' casting issue. I see there are already a few good points on this. I think the golden rule is that you should know how both types of casting works and then it becomes easy to figure out which one you want to use. I've seen a presenter at a conference use 'as' when casting without checking for null, so this is clearly a common issue.

1/25/2011 5:16 AM | Jaco Pretorius

Comments have been closed on this topic.

Powered by: