Author’s Note: It appears in the conversion from BlogEngine.NET to WordPress the code block was truncated. I have no idea what it used to read; but if I can find it I’ll edit it in. Otherwise, assume it was horrid code.
Recently I was cleaning up some code in a legacy application and found this little gem:
private IList entities = new List
This code was written about 2005. C# had the ‘using’ block, as well as try/catch/finally. But in this case, neither is used to clean up the dataAdapter .
That’s important to know. It’s really easy for me to cast judgement on this code; but what really matters is *why* this code was written the way it was. It’s not as if the people that worked on this application were stupid — on the contrary they were probably quite smart. There are a number of reasons why it was written this way:
- Programmer didn’t understand what could happen to the open connection if something failed.
- Programmer didn’t realize there was a ‘using’ or ‘finally’ statement in C#.
- No Code reviews (or)
- No one senior enough to review the code.
There’s a certain amount of ‘If it isn’t broke, don’t fix it’ in legacy applications. There’s inertia against changing code, especially if the code physically works. But the real question is, Should the code be changed? and the answer is unequivocally Yes.
What happens if it bombs when calling FillSchema? What about if the connection to the database dies? What about if it throws an exception for some other reason?
These are all questions we should be asking ourselves when working on code. I hate to invoke Murphy, but if there’s a chance that something can go wrong, it will. It’s our job to make sure that when it does go wrong, we handle it gracefully.
Don’t ever accept We’ve always done it that way as a valid reason to keep code the way it is. Code is a living document, not the Constitution. Treat it as such.