Printable View
Hi, I'm trying to come up with a good set of C# coding guidelines for my team. What I'm interested in are things that could could prevent actual problems or optimizations rather than just coding preferences like using tabs vs spaces or naming conventions...
If anyone has any good ideas or links to good C# coding guidelines, I'd like to see it.
Here's some things I came up with so far:
1. Use proper Dispose pattern to properly release resources (such as raw pointers or handles, files or network connections…):
Code:class SomeClass : BaseClass, IDisposable
{
SomeManagedResource _managedResource = null;
SomeUnmanagedResource _unmanagedResource = null;
bool _disposed = false;
public void Dispose()
{
Dispose( true );
GC.SuppressFinalize( this );
}
protected virtual void Dispose( bool disposing )
{
// If you need thread safety, use a lock around these
// operations, as well as in your methods that use the resource.
if ( !_disposed )
{
if ( disposing )
{
// Clean up managed resources here.
if ( _managedResource != null )
{
_managedResource.Dispose();
}
}
_managedResource = null;
// Clean up unmanaged resources here.
if ( _unmanagedResource != null )
{
ReleaseHandle( _unmanagedResource );
_unmanagedResource = null;
}
// Always call Dispose() on all disposable base classes.
base.Dispose( disposing );
// Indicate that the instance has been disposed.
_disposed = true;
}
}
~SomeClass()
{
Dispose( false );
}
}
2. Call the Dispose() method of all classes that implement IDisposable in try/finally blocks or in ‘using’ statements rather than waiting for the garbage collector to do it for you because then it will free up those resources sooner and garbage collection may happen a long time later (if ever).
3. Don’t use Sleep() as a hack! Minimize the number of places you use it and only sleep for the shortest time necessary. An example of using Sleep() as a hack is when you launch an asynchronous process and you want to wait until the process is finished before continuing. You determine that the process takes about 10 seconds to finish, so you add Sleep( 15000 ) and then continue the rest of the program.
This is very sloppy and dangerous because there are many things that could cause the process to take longer than 15 seconds, in which case your program would fail because it’s assuming the process is finished. It also wastes time because on some systems the process may only take 5 seconds to finish, in which case you’re doing nothing for 10 extra seconds. A classic symptom of programs that use Sleep hacks is intermittent “flaky” failures.
The proper way to handle a situation like this is to use a loop and only sleep for a small interval each iteration (such as 100ms) then check a condition that would indicate whether the process is complete, then break out of the loop.
4. Don’t hard-code configuration data. Put things like paths & filenames, hostnames, port numbers, timeout values, usernames/passwords, logging levels… into separate config files or a common file included by all your source files. This way it’s easier to modify those values later since they’re all in one place. The only hard-coded strings you should put in your code are things that won’t need to change based on different configurations, such as error messages.
5. Use System.IO.Path.DirectorySeparatorChar instead of hard-coding a ‘\’ or ‘/’ in directory paths; and use relative paths instead of absolute paths whenever possible. This helps keep your code portable between Windows & UNIX.
Here's some things I came up with so far:
1. Use proper Dispose pattern to properly release resources (such as raw pointers or handles, files or network connections…):
Code:class SomeClass : BaseClass, IDisposable
{
SomeManagedResource _managedResource = null;
SomeUnmanagedResource _unmanagedResource = null;
bool _disposed = false;
public void Dispose()
{
Dispose( true );
GC.SuppressFinalize( this );
}
protected virtual void Dispose( bool disposing )
{
// If you need thread safety, use a lock around these
// operations, as well as in your methods that use the resource.
if ( !_disposed )
{
if ( disposing )
{
// Clean up managed resources here.
if ( _managedResource != null )
{
_managedResource.Dispose();
}
}
_managedResource = null;
// Clean up unmanaged resources here.
if ( _unmanagedResource != null )
{
ReleaseHandle( _unmanagedResource );
_unmanagedResource = null;
}
// Always call Dispose() on all disposable base classes.
base.Dispose( disposing );
// Indicate that the instance has been disposed.
_disposed = true;
}
}
~SomeClass()
{
Dispose( false );
}
}
2. Call the Dispose() method of all classes that implement IDisposable in try/finally blocks or in ‘using’ statements rather than waiting for the garbage collector to do it for you because then it will free up those resources sooner and garbage collection may happen a long time later (if ever).
3. Don’t use Sleep() as a hack! Minimize the number of places you use it and only sleep for the shortest time necessary. An example of using Sleep() as a hack is when you launch an asynchronous process and you want to wait until the process is finished before continuing. You determine that the process takes about 10 seconds to finish, so you add Sleep( 15000 ) and then continue the rest of the program.
This is very sloppy and dangerous because there are many things that could cause the process to take longer than 15 seconds, in which case your program would fail because it’s assuming the process is finished. It also wastes time because on some systems the process may only take 5 seconds to finish, in which case you’re doing nothing for 10 extra seconds. A classic symptom of programs that use Sleep hacks is intermittent “flaky” failures.
The proper way to handle a situation like this is to use a loop and only sleep for a small interval each iteration (such as 100ms) then check a condition that would indicate whether the process is complete, then break out of the loop.
4. Don’t hard-code configuration data. Put things like paths & filenames, hostnames, port numbers, timeout values, usernames/passwords, logging levels… into separate config files or a common file included by all your source files. This way it’s easier to modify those values later since they’re all in one place. The only hard-coded strings you should put in your code are things that won’t need to change based on different configurations, such as error messages.
5. Use System.IO.Path.DirectorySeparatorChar instead of hard-coding a ‘\’ or ‘/’ in directory paths; and use relative paths instead of absolute paths whenever possible. This helps keep your code portable between Windows & UNIX.
As a note for Sleep: Don't. Simple as that. Don't. All threading features in C# have a way to call you back when they are finished. Sleeping is an error, plain and simple. Even sleeping in a loop waiting for a condition is bad programming.
You might want to look into static code analasys in Visual Studio (former FxCop) and the StyleCop Plugin for R#.
Can you explain why something like this is bad?Quote:
Originally Posted by nvoigt
Oh, and maybe I should clarify what kind of code my team writes. We do QA Automation, so we write automated tests for production software. Maybe that would have an effect on some of the coding guidelines I should be recommending?Code:// psudo-code
proc = Process( "some_test.exe" );
while ( proc.IsRunning() )
{
Sleep( 100 );
}
// Collect test result...
Also, I'm pretty new to C# (which is why I'm looking for tips on best practices), but I really liked the guidelines Herb Sutter had in "C++ Coding Standards: 101 Rules, Guidelines, and Best Practices".
I'm wondering if some (or most) of those guidelines would apply just as well to C#? Obviously anything related to memory management/smart pointers, STL, and a lot of the template stuff wouldn't apply to C#.
In C# there is always a way to wait for a thread or process. Looping and polling is always the worst way to spend time. There is either callbacks or signals or continuation tasks or a million other ways to wait for a signal that some other parallel process has ended.
There's guidelines for developing C# here. Most C++ guidelines focus on C++ (obviously) and while generic guidelines are always a good thing, just taking some from C++ is a recipe for disaster because the languages function differently and good practice for one of them might be bad for the other. You will need an own set of guidelines for C#.Code:var p = new Process { StartInfo = new ProcessStartInfo("calc.exe") };
p.Start();
p.WaitForExit();
Hey, I just used that WaitForExit() today actually. ;)
I guess my example wasn't that good. But here's a better one. Lets say you're starting a service on Solaris that takes a while before it's completely started, like Jenkins, but you want to wait until it's all the way up before continuing your program.
will return with exit code 0, but if you try to load Jenkins in a web browser it gives you a message that it's still starting up.Code:svcadm enable -st jenkins
I think this is a better example of when you'd need to use a Sleep loop and check a condition such as whether you can connect to a certain port or something.
there is no exception to the rule that every rule has its exceptions.Quote:
Originally Posted by cpjust
Should be "Use proper Dispose pattern to properly release unmanaged resources (such as raw pointers or handles, files or network connections…):Quote:
Use proper Dispose pattern to properly release resources (such as raw pointers or handles, files or network connections…):
In all honesty the Dispose pattern is a massive Microsoft hack job to cover up a deficiency in the language and to make it appear as if you have deterministic object cleanup when you do not. The only way the pattern works is if everyone follows the rules. There is no way to enforce the pattern and as such it feels like a giant hack.
I would go with the suggestion of using StyleCop. It will enforce the style guidelines and rules that you want. Past that you can use ReSharper which is very handy albeit a bit wordy at times. Thankfully you can configure it to shutup about certain things it finds.
A few annoyances about the default settings in ReSharper:
- It will make you think you have to use var everywhere which IMHO is simply bad practice and not needed
- It tends to bring typing to a crawl in the XAML editor when doing WPF. Known issue and I believe they are still working on it.
Other than that it not only offers a host of new features for MSVS but it has very good analysis tools as well. Past that you can use FXCop which will perform static analysis on your code.
StyleCop, ReSharper and FXCop will clean up your code base fairly quick. If you start with these then you will maintain a fairly tidy code base. Remember to treat warnings as errors for StyleCop...this will ensure that developers cannot simply ignore the StyleCop suggestions. Developers that do will check in code that does not compile and will quickly become unpopular amongst the team. At first StyleCop will be a PITA but once you learn what it likes and dislikes you will begin to write code that is accepted on the first pass. All in all I believe it creates a much tidier code base and sync up developer code styles to produce a consistent style across the team and/or company.