I saw this list of “Basic C# coding guidelines” and I started off just replying to it in a comment, but there are so many that I disagree with, that I’m going to do it in a whole separate post.
I’ll answer all the ones I disagree with, if I didn’t include one (and there’s only 2 that I don’t include!) then it means I agree with it. So let’s go!
Please do NOT use + sign to concatenate strings, because it creates three instances of string (try doing that in a long or infinite loop, your program will die with OutOfMemory exception in no time), Rather use string.Concat it keeps one instance (and hey no OOM). If concatenating many strings in a loop etc, then use StringBuilder. Alternative to string.Concat is string.Format (which uses StringBuilder inside)
This completely unqualified statement is a bit misleading, first of all. If you’re just going to do a:
string a = "hello " + "world";
Then string.Concat will not have any advantage, and also suffers from the disadvantage of being harder to read. If you’re concatenating more than one string, then perhaps string.Concat would be better (but usually string.Format is better in that particular case).
In any case, a blanket “do not use + operator to concatenate strings” statement doesn’t really tell us much.
If using Generic types, then refrain from using foreach loop on the collection. Rather use ForEach method to loop through via an anonymous method predicate (much faster because doesn’t create the Iterator).
The ForEach method, while it doesn’t allocate an iterator, obviously does allocate a delegate. Also, calling a delegate for each object in the collection, as opposed to advancing the iterator, is probably slower anyway (given that advancing the iterator – for most collection classes – would be inlined).
Again, this also comes back to readability. And that’s the whole purpose of foreach – it’s 10 times more readable to use a foreach loop than to pass a delegate (even an anonymous one) to the ForEach method. I mean, come on – if allocating the iterator is any more than an insignificant dot on your performance radar, I’d be very surprised.
Nullify unused objects (doesn’t collect, but marks for collection and ceases from getting promoted into next generation)
Again, this is a blanket statement that if used in all contexts can result in worse performance. While it is true that the jitter cannot determine if member variables will be used again in the scope of a class, the jitter does an excellent job of determining when local variables are no longer user, even if their value is not set to null.
In fact, setting a local variable to null will artificially increase its lifetime, since the variable is still being “used” when you set its value to null.
In certain, limited scenarios setting a member variable to null may cause it to be collected earlier. However, it again comes down to readability vs. impact on performance. Because garbage collection is unpredictable, you don’t know whether the GC will kick in anyway and an extra “var = null” just makes things worse.
Besides, if you come in later to modify the code, and you want to access the variable later on in the function, you’d better remember that you’ve already set to null earlier on!
IF conditions having just one item in if and else, should be used as ternary operator (? Sign)
This is a total syntactic-sugar change. There is no difference in the generated code between an if...else block and a ?: statement. The only difference is that ?: is an r-value. It might save you an extra assignment instruction.
Personally, I believe the exact opposite is a better approach – change instances of ?: with the corresponding if...else block. It increases readability no end – especially for complex conditions.
Use ‘as’ operator instead of direct typecast using parenthesis with the exception of overloaded explicit cast operator, it saves from NullReferenceException and InvalidCastException
An explicit cast will not generate a NullReferenceException. However, there are certainly times when an InvalidCastException is desirable. This is another “blanket” statement that is simply not true in significant numbers of circumstances. Generally, the as operator is best used as a replacement for is+cast, i.e. replace:
if (foo is MyClass)
{
MyClass myclass = (MyClass) foo;
// do stuff
}
With:
MyClass myclass = foo as MyClass;
if (foo != null)
{
// do stuff
}
This is better because there’s really only one typecast going on. But again, if this is affecting your actual performance (outside a tight loop, anyway) I’d be surprised.
Refrain from XmlDocument usage for navigational purpose, please either use XmlTextReader for sequential access or XPathDocument for XPath based data retrieval
This was true in early betas of version 2.0 – XmlDocument was not modified much for performance. Much work was done later on to increase its performance, though, and today it is quite acceptable to use XmlDocument where appropriate (i.e. don’t use it on 100MB XML documents, but that should be obvious).
For server side XSL transformation, Use XslCompiledTransform instead of XslTransform (Please check http://blogs.msdn.com/antosha/archive/2006/07/24/677560.aspx).
This is another blanket statement that really should come with a number of disclaimers. First of all, using XslCompiledTransform generates a .NET assembly and loads it into your process. If you’re going to be doing thousands of different XSL transforms, then use XslTransform (which may be quite likely in a server-side process). Also, because it’s compiled, it has significant first-use overhead (to compile the transform). If you’re only going to use the XSL file once or twice, use XslTransform.
I don’t know much about doing XSL in a browser, so I won’t comment on the second half of that statement.
Always join your threads in a web page, if used (otherwise the page will be rendered and workers will continue operating at the server cost, and ofcourse the results will be unpredictable)
I would qualify this with “don’t use threads in a web page.” Certainly not in any way that would require that your join with them before ending the request!
Always do NULL checking before operating on an object, NullReferenceException is widely occurring exception in most applications and only a preventive programming can help us get the real error
This is rather nonsensical. If you do null-checking before using a parameter, what are you going to do? All you can really do, if the null is unexpected, is throw a NullReferenceException anyway! This should probably also be qualified with a “in a public interface method.” There’s no point (and plenty of harm) in doing null-checks in internal and private methods.
Conclusion
The problem I had with this list, and with most “best practices” lists in general, is that it’s filled with blanket statements that should really come with plenty of qualifications. After all, if the + operator was so bad on string objects, it wouldn’t have been added in the first place. If XslCompiledTransform was so much better than XslTransform, then XslTransform would have been marked “deprecated.” And so on.
I especially have problems with “performance” guidelines, because there are no hard-and-fast rules about getting code to perform well. If replacing all instances of foreach over arrays with for-and-an-index would have any effect on the performance of your application as a whole, then you couldn’t be doing anything inside the loop of worth!