In 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 }
287
288 boolean fSwapped;
289 DNSResourceRecord rrSwap;
290
291 do {
292 fSwapped = false;
293
294 for (int i = 0; i < arr.length - 1; i++) {
295 boolean fSwap = false;
296
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 }
306
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 }
316
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).