Programmer, heal thyself

heal-thyselfimgIn my quest for unclear/confusing/just plain bad code, I couldn’t in good conscience ignore my own code. Of course, code I’ve written recently is still too fresh in my mind to be looked at objectively. I would probably still see things now the way I did a few months or a year ago.

But what about code I wrote five years ago? Or more than ten years ago? To test my theory, I’ve started digging up some development projects from the archives so that I can see how my style has changed and also see if maybe some of my code can be of use to someone. I’ve created a new Programming section of my site and I’ve posted up a pretty simple example of my Java programming circa 1997.

The example I’ve chosen to start with is a set of classes for doing DNS queries. It was originally written for a SMTP/POP3 server I wrote in Java. Maybe if I get ambitious I’ll start posting some of that up as well.

There are many things that I’d do differently if I were writing this code today, but I’ll pull out one obvious example to talk about:

283   private void SortRRs(DNSResourceRecord [] arr, boolean fDescending) {
284     if (arr == null || arr.length < 2) {
285       return;
286     }
288     boolean fSwapped;
289     DNSResourceRecord rrSwap;
291     do {
292       fSwapped = false;
294       for (int i = 0; i < arr.length - 1; i++) {
295         boolean fSwap = false;
297         if (arr[i+1].m_iType < arr[i].m_iType) {  
298           fSwap = true;
299         } else if (arr[i+1].m_iType == arr[i].m_iType) {
300           switch (arr[i].m_iType) {
301           case TYPE_MX: {
302             fSwap = arr[i+1].m_lData < arr[i].m_lData;
303           } break;
304           }
305         }
307         if (fSwap = fSwap ^ fDescending) {
308           rrSwap = arr[i];
309           arr[i] = arr[i+1];
310           arr[i+1] = rrSwap;
311           fSwapped = true;
312         }
313       }
314     } while (fSwapped);
315   }

What this subroutine is doing should be obvious to most old-school programmers. Yes, it’s the infamous bubble sort. It’s widely regarded as one of the worst types of sorts available. So why did I use it? Probably because it was easy to remember and I couldn’t be bothered to create the extra class I would have needed to use the built-in Java sort()method.

307         if (fSwap = fSwap ^ fDescending) {

When I wrote this code, I was obviously in love with my knowledge of Boolean logic and operator precedence. I wrote this code and it actually took me a minute to decipher what it really meant. Here are some mistakes I made in this case:

1. Inaccurately naming my variables. ThefSwapflag actually should be called thefBiggerflag. It doesn’t necessarily mean that a swap should happen, it just means that the current array element is “bigger” than the next one.
2. Using obscure operators. Yeah, yeah, everyone should know their XOR truth table, but it would have been much easier to read if I had written(fBigger != fDescending). Not as “cool”, but much easier to decode after 11 years.
3. Unnecessary assignment. I assign the result of the XOR back into thefSwapvariable, and promptly forget it. Removing the assignment would have made the logic clearer and saved the initial confusion that some programmers have when they see an assignment in a conditional expression (thinking that the programmer meant == when he really meant =).
All that from one little line of code. So had I written it today, it would have looked something like this:

if (fBigger != fDescending) { .. do swap }

In the long run, overly-clever code really doesn’t save us much. If you don’t believe me, consider what the father of the quicksort algorithm said:

We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil.

–C. A. R. Hoare

For our next installment, I’ll probably be tackling one of my other older Java projects, possibly the mail server, or even my Java-based COBOL compiler (it’s a long story).

Leave a Reply

Your email address will not be published. Required fields are marked *