You are viewing t3knomanser

t3knomanser's Fustian Deposits

"Breathtaking" code

How Random Babbling Becomes Corporate Policy

IOCCC Original Winner

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.
  • There is a class called Utility, which does logging, encryption, and data type conversions (because they couldn't use the native functionality for this for some reason). And yes, it does encryption, not one but TWO ways (ROT13 and AES- I think only the AES is actually used), but it doesn't encrypt passwords.

    The other class with a kitchen-sink set of functionality is actually the parent form. The app is an MDI application, and the main form contains all the data access and business logic. All of the other forms have to constantly call out to that form to actually get stuff done.

    And yes- on first run, if it fails to create the columns, in just keeps trucking along merrily. The real irony is that this guy also wrote a DB converter to uplift the Access DB into SqlServer, but apparently didn't have IT create the columns.

    Oh, also, the way he attempted to implement transactions was by sending "BEGIN TRANSACTION" to the database, instead of using the actual transaction functionality. Needless to say, it didn't work, so he actually had a trap in his "ExecuteNonQuery" procedure that said, "If the SQL code is 'BEGIN TRANSACTION' do nothing."

    That's right: there's no transaction isolation.

    In any case, there are times where a Utility class is just a convenience, but in general it's a bad practice. I'm now on the review committee that I dragged this app through the first time, and I've been pressuring developers to NOT create Utility classes.

    Edited at 2009-10-14 01:44 am (UTC)
  • This sounds like he might have run the project update wizard on the original VB6 code and made the bare minimum of manual changes to make it work under .NET.
  • The update wizard was probably involved, but there's a lot of obviously new code. I think that might have been his plan, and then he discovered that the upgrade wizard sucks, and that he had to do a lot more work.
  • Probably more work than if he had done a clean rewrite, using the original as a functionality reference only.

    I've used it a few times on code I understood, and been baffled at what it spit out at me. I suppose if the original code is extremely well written(not me- I can avoid the brain dead stupidities but I'm sure a pro could find some horrors in what I've written) it might produce useful results sometimes. For the most part though, all it does is make your Visual Studio program directory fill more disk space.
  • There's not a lot of well written VB6 code. One of the main complaints people generally have about VB is that, before .NET, it made it easy to write bad code and hard to write good code. It helped for RAD, but made it suck for maintenance.

    I've run the conversion tool on some simple projects, and unless the project is utterly trivial, it just doesn't give you good results. And you get none of the benefits of migrating to .NET, in terms of OOP and reusable code.

    Awhile back, my organization was asking me what role the converter could play in upgrades, and my response (which they didn't like), was that it should only be used for applications we wanted to kill, or at least, didn't expect to be doing much maintenance on in the future.
  • I found QBasic easier to write somewhat OK code in than VB. Sure, it was all in one source file for the most part, but you weren't encouraged to so directly mix user interface and functionality.

    Interesting side note, for my computer logic course, we used QBasic. Mainly so the professor didn't have to actually teach a language, he could focus on the logic. I was the only one to actually use subroutines in the final project.

    On the other hand, I was also the only one to use GOTO. I did have some sanity, only jumped forward, and the label was "ENDLOOP" to clearly indicate what the goto was for. I wasn't exactly happy to use a GOTO, but I had no time left to finish and I wanted the extra credit for including file I/O.
  • "VB" in the first paragraph should say "VB6"
Powered by LiveJournal.com