Unhygienic .NET code

I’m currently rewriting an ASP.NET page that’s part of a large and very busy website. The code is so bad it’s difficult to imagine how it ever worked in the first place, let alone serviced hundreds of thousands of hits every day. I’m refactoring it to run within a new architecture that has proper boundaires between presentation, business and data layers. It’s a close call whether or not to completely rewrite the page from scratch, but there’s too much danger of missing some crucial functionality.

Here are a few points I’ve picked out that would’ve made the original code less disgusting.

1. Don’t build your entire page’s logic into a single Page_OnLoad event handler. This routine should only contain calls to other functions that need to be executed when a page loads.
2. Comment your code, even if you just add markers to roughly indicate what each group of statements is doing. It’s painstaking to trawl though thousands of lines of uncommented code trying to work out what each bit does.
3. Modularise your code. Split each separate unit of work into its own function or subroutine. Pass data to these routines in parameters rather than sharing data in global or other broad-scope variables. Make your routines loosely-coupled and highly-cohesive.
4. If you use a data layer, as you should be, don’t pass an open SqlDataReader (or any DbDataReader) to your presentation layer, the .aspx code-behind. Your data access should be abstracted from the presentation and business layers, so they aren’t tied to a particular implementation. I like to use business entity objects and generic collections, but you can also use strongly-typed datasets or XML. The pros and cons are outlined in this MS Patterns & Practices guide, Designing Data Tier Components and Passing Data Through Tiers.
5. Don’t use Hungarian notation, where the variable name indicates the data type, such as intRowCount instead of just rowCount. This was useful for weakly-typed languages that use variant data types, but is useless and messy in .NET’s strongly-typed languages. If you try to do something not supported by a particular data type then it won’t compile. You can find out a variable’s type by hovering the mouse over it in Visual Studio.
6. Don’t declare all your variables at the top of a function, especially when your function is several thousand lines. It’s not 1980 and this isn’t C. Keep your variables’ scope as small as possible, and declare them near where they’re used.
7. Don’t open multiple database connections on the same page an leave them open until the very end. You shouldn’t be opening any connections anyway, this should happen in the data layer, but if you must then at least only open them where they’re needed and close them as soon as you’re finished. There’s a large overhead involved in keep database connections open, so this mistake can seriously damage your application’s performance.
8. Don’t wrap all your code in a massive try block with an empty catch that swallows up any exceptions that might be thrown, just because you can’t be bothered to do any error handling. Exceptions should either be handled or propagated up the call stack until they are.

Remember, the next person who has to maintain or extend your application might be you. Or even worse, me.