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