Log in

No account? Create an account

t3knomanser's Fustian Deposits

"Breathtaking" code

How Random Babbling Becomes Corporate Policy

run the fuck away

Mad science gone horribly, horribly wrong(or right).

"Breathtaking" code

Previous Entry Share Next Entry
IOCCC Original Winner
Words like "breathtaking", "stunning", "astounding", "amazing", and "incredible" are not positive words. Oh, they're often used in a positive context, but in reality, they only convey a sense of wonder and disbelief. They can be applied to something horrifying just as easily as something delightful.

Such has been my experience over the past few days.

Once upon a time, some fragment of the giant behemoth I work for needed an application to manage some inventory tracking and similar tasks. They found an external contractor that would make it for them. In that era, the app was built in VB6 with an MS Access back end. Horrible, yes, but hey, it was the 90s.

Well, now it's time to upgrade. So they go back to the same contractor. "Hey, we want it to be .NET and use SQL Server. Here's a check."

Well, it wasn't that easy, of course. There are policies. Checkpoints. Oversight. I was part of the oversight, and my job was to get the application through our "gates". Since this was a purchased application, we didn't care if it was terribly well designed, so long as it worked. We weren't going to have to maintain it. So we got some design diagrams from the contractor, sanity checked them, and then called it a day.

More fool I.

Last week, I hear, "Hey, that application- they're not satisfied with the support from the vendor. They want to bring it internal. Hey, Remy, find out what you need to do to make it fit with our standards."

Now, when we develop an app internally, or have a contractor develop an app that we intend to support, the auditing requirements are much stricter. We don't care if an outside vendor has to suffer under horrible support issues, but if we're wasting time on stupid support work, we're unhappy.

So, the first step then, would be to audit the application via a code review. I sat down in a room with three other developers, we pulled up the code, and we started skimming through it.

It was breathtaking.

The first, most glaring thing, was that the actual design and the provided diagram had no relationship to each other. That didn't bode well. And it was all downhill from there. As a sampling of some of the amazing things we saw:
  • One window in the application has all of the business and data access code bundled up in it. It's like they took all the functions and dumped them into a bucket.
  • But not all functions. For example, when that main window wants to log an error message, it calls out to a class called "Utility". When it does that, the Utility class makes a call against the main window. Not only is the code just a disorganized pile in a big bucket, but sometimes the bucket says, "Hey, you gotta go look in another bucket to find that!" and when you do, the other bucket says, "Nope, it's actually back out in the original bucket."
  • All of the database access stuff is hard coded strings. With no SQL injection protections.
  • It stores passwords in plaintext
  • The most stunning thing, however, is what the application does every time it launches. Every time the application launches, it attempts to add columns to a bunch of tables. If those columns already exist, this would cause an error, so it just ignores the errors. Since, once the app has run once, the columns will exist, that means every time a user runs the app, it's trying to add columns to tables for no damn reason.
There's more, but I won't bore you with the details. I wrote a 2,200 word code review document. Normally, these documents follow this form: "File X, Line 128, variable strFoo should be named foo, we don't use Hungarian Notation." For even large projects, they don't tend to get that long, just because they're so terse. This, this was a 5 page essay. I couldn't even call out flaws by line numbers, just because the developer was so clearly incompetent. I've dealt with some bad code in my day, but this is just special.

There's a punchline. They started development using .NET 1.x, and then partway through switched to .NET 2.0. In .NET 1.x, there was no built in FTP functionality. If you wanted to do FTP, you needed to write your own class to do it, or steal some. Microsoft's developer documentation site, MSDN, had an MSFTP code sample, which demonstrated how one could implement their own FTP code. The developer copypasted this- despite the fact that .NET 2.0 had all the functions he needed- and included a class called "MSFTP" in his project.

He couldn't just use the class as it was, however. There were some quirks in our FTP server it didn't handle. And while he was at it, he added a "Dispose" method to handle cleanup, replacing Microsoft's use of the "Finalize" method. This is actually a good .NET technique for technical reasons that are irrelevant here, so he obviously read a book. He didn't read it closely enough, because his Dispose is actually implemented wrong, but that's neither here nor there.

In the comment above his "Dispose" method, he had the gall to include this:
'M$ should have implemented this. Man, this code is sloppy.

When I read that, I just started cracking up.

Until I saw how much my company paid for this. And then I started crying.
  • You should have just sent them a memo that read "Much more work is needed before this software is ready to enter the closed beta test stage."
Powered by LiveJournal.com