In my previous post I highlighted a single bad idea that I had seen in some code. I want to offer up a few other things that I saw and why you shouldn’t do them…
- Tightly nested Try/Catch blocks — basically, put a try block, do one statement, add a try block in that block and repeat… See the code…
static public void LogRequestResult(string url, string status)
{
try
{
FileStream objFileStream = null;
StreamWriter objStreamWriter = null;
try
{
objFileStream = File.Open(cnstMyFilePath, FileMode.Append, FileAccess.Write, FileShare.Read);
try
{
objStreamWriter = new StreamWriter(objFileStream);
lock(objStreamWriter)
{
objStreamWriter.Write(Status + url);
}
}
finally
{
if (objStreamWriter != null) objStreamWriter.Close();
}
}
finally
{
if (objFileStream != null) objFileStream.Close();
}
}
catch
{
}
}
Ignoring the fact that we’re not actually doing any error checking… Isn’t the following much more straightforward easier to read, and less prone to errors? The moral to the story, try/catch blocks are cool but don’t abuse them.
static public void LogRequestResult(string url, string status)
{
FileStream objFileStream = null;
StreamWriter objStreamWriter = null;
try
{
objFileStream = File.Open(cnstMyFilePath, FileMode.Append, FileAccess.Write, FileShare.Read);
objStreamWriter = new StreamWriter(objFileStream);
lock(objStreamWriter)
{
objStreamWriter.Write(Status + url);
}
}
finally
{
if (objStreamWriter != null) objFileStream.Close();
if (objFileStream != null) objFileStream.Close();
}
}
-
Magic Numbers (or in this case letters), just stop — I can’t tell you how many times I’m looking through code and I come across an if statement that looks like “if myvariable == “A““ Why in the world would your variable is “A“. What does “A“ mean? In .NET we have enumerations and constants. Why can’t we just them? I have no idea what “A“ means — and neither will the author 2 months from now. If you put quotes around the same text twice it should be a constant. If you use a number for an if statement or as part of an assignment that isn’t a 0 or a 1, you should have a constant. (Zero is a number to compare against, 1 is a number to fix a number of offset issues.)
-
Comments — Hey, there’s a concept. And I don’t mean one that says, // the following line adds one to the number. Thanks. I can read code. How about writing comments that tell me why you’re adding one to the number? Oh, yea, those XML comment things that .NET supports. Those are useful. You can use them too, they’re not expensive.
Do you have any pet peves about code that should go on this list?
