For those of you who played D&D (here's a funny video to see what it's like), you might recall that there were magical tomes that could increase or decrease your abilities, just by reading them. Of course that's impossible in real life since we'd need powerful magic... right? Well, as I have unfortunately learned, no. A while ago, I had to maintain someone else's app. I believe in the process of reading this app's code, I have lost a few IQ points. Let's take a look, shall we?*

All the code in this app uses horrible variable names. In a 250 line block of code (a single method -- the writers must have thought there to be huge drawbacks to using methods), the first line starts off by declaring the variables. A sample looks this:

dim objconn,objrs,strDatabase,mysql,mysql1,sqlstring,rstemp,dbConn1,objrs1,query

This is a truncated line. They actually declare about double that much. Regardless on how you feel about declaring everything at the top of a file, this is bad. They don't use these variables at the same time. For instance, they'll open objrs, do something, and then close it, then open rstemp and repeat. There aren't actually two objects in use at once. They just declared extra variables for fun. Or maybe they thought they had to give the variables a rest. I don't know. And I don't think they did either. Of course, it's better than using no variable names at all.

They have a process to read values from a comma-delimited file. So, one line at a time, they use VB's split function, storing the result in a variable named “split“. So far so good. Then they proceed to use constants for the next 100 lines to refer to different fields, giving way to wonderful code as so:

if split(6) = “true“ then
  objrs1.open “SELECT * FROM Table WHERE Field1 = “ & split(2) & “ Field2 = '“ & split(9) & “'“
  split(4) = objrs1(“SomeField“)

At a few places in the app, a field is selected from the DB for absolutely no reason:

someId = Request.QueryString(“someId“)
rs.Open “SELECT SomeId FROM Orders WHERE SomeId = “ & someId, objConn1
someId = rs(“SomeId“)

That's right. They select a single field (an int), constraining it to the current value of their var, and then set the var to the same value. Maybe there's something special in SQL that I'm not aware of. To their credit, there's actually a check for rs.Eof first (omitted for clarity of stupidity).

Here's a brilliant idea for performance: Don't use SQL's COUNT. In quite a few places, they'll execute a semi-complex query that returns, on average, 10,000 rows. But why bother with SELECT COUNT, when we have SELECT *?

The entire app is built like this. The people who wrote this should have their text editors confiscated.

* Some variable names have been renamed to protect the innoce-- mentally challenged.