Boy, was I astonished to see the code fragments below in production apps. It is far more Daily WTF than Coding Horror.
However, it did make debugging the production problem at hand a lot worse than it should be.
First a few short pieces:
private void method(Word.Application objNewDoc, string stringWithCsvSeparatedDotFileNames)
{
char c = char.Parse(",");
string[] wordAddIns = stringWithCsvSeparatedDotFileNames.ToString().Split(c);
}
It took me almost a minute to understand what happened here. Especially because the names of parameters actually were pretty meaningless.
foreach (string sFilename in attachments)
{
Word.Application mailDocument = new Word.Application();
string[] filePath = sFilename.Split('\\');
string tempDirectory = GetTempDirectoryFromConfigFile();
object fileName = tempDirectory + filePath[filePath.Length - 1];
File.Copy(sFile, (string)fileName, true);
// some code that actually does something with the attachment
}
It took me more than a few minutes to realize that:
- The tempDirectory needs to end with a backslash
- mailDocument (not a document, see below), will stay alive when File.Copy(…) throws an exception.
internal virtual bool Method(/* parameters */, Word.Application objDoc)
{
// lots of pre-initialized empty string variables that are used at the very end of the method
Word.Application objNewDoc;
if (objDoc != null)
{
objNewDoc = objDoc;
}
else
{
objNewDoc = new Word.Application();
}
// lots of Object variables for COM, including:
Object missing = Missing.Value;
Object bFalse = false;
try
{
// lots of code that does not use objNewDoc
}
catch (IOException IOex)
{
objNewDoc.Quit(ref bFalse, ref missing, ref missing);
objNewDoc = null;
throw IOex;
}
catch (Exception ex)
{
objNewDoc.Quit(ref bFalse, ref missing, ref missing);
objNewDoc = null;
throw new Exception("Some error message.", ex);
}
finally
{
// empty finally block
}
try
{
// actual coder that does use objNewDoc
}
catch (Exception ex)
{
objNewDoc.Quit(ref bFalse, ref missing, ref missing);
objNewDoc = null;
throw new Exception("Some error message.", ex);
}
return true;
}
I rewrote the whole piece into separate methods.
Luckily the person who wrote this got promoted away from programming a few years ago.
–jeroen
