Assumptions and Legacy Code

Yesterday I encountered an interesting snippet when going over some legacy code that really made me think about my assumptions.

if (ipAddress.Length > 15)
{
    ipAddress.Substring(0, 14);
}

This should be pretty clear to most folks, if the IP address string is longer than 15 characters, truncate it to 15. Theoretically, the longest an IPv4 address could be is 15 characters (000.000.000.000) if written in base 10.

Determining Intent

Why would we ever need to truncate it to 15 if this is the case?

One of my colleagues raised a good possibility: what if that IP address contained additional information, such as a port number? It could then appear as 000.000.000.000:00000. That makes some sense, but what about the many cases where each octet (group of 000) is less than three digits? In that case some of a port number may be included in the resultant string and it wouldn’t make a whole lot of sense to do that.

Besides, if you just wanted to drop the port number, why wouldn’t you use regex to trim the string properly? Not that I love regex, but it does have its uses.

“I had a problem, so I used regex. Now I have two problems.”

Another idea surfaced from a different colleague (don’t odd pieces of code attract developers like flies to honey?): maybe the actual value of the string is unimportant and the database is limited to 15 characters? That also wasn’t the answer.

It seems that if you wanted to get an IP address from a string in .Net your best choice would be to use IPAddress.Parse() which will parse IPv4 and IPv6. If that didn’t work because you had port information, you could parse it as a URI and extract the IP address that way.

It’s also worth noting that this snippet of code contained no comments and has no unit tests. There’s no real way to extract the intent from the code. This is a perfect example of a spot where a comment could indicate intent to future developers for maintenance. It’s also a perfect example of how well-defined unit tests can help provide intent in the same way, while also showing whether or not it is safe to alter the code.

Assumptions

You might have noticed that this post has “Assumptions” in the title, but so far I haven’t discussed any.

There are two possible assumptions that one could make here:

  1. Clearly the person who wrote this code had no idea what they were doing.
  2. The person who wrote this code is smarter than I am and wouldn’t have written this without a good reason.

(Ok, there are really a lot of other possibilities for assumptions, but these are the two I flip between.)

In the case where you are working on a defect, and the code in question seems to be the problem, the first assumption may be a safe bet. Really though, even then you must assume that you can’t just rip the code out and instead figure out why the code doesn’t work as intended, meaning you must still determine the intent.

In the case where I am just cleaning up code that works, the second assumption is always my choice. I am not aware of any defects caused by this section of code, which is not to say that they don’t exist. So it is far safer to assume that someone put this code in to resolve a defect. They likely believed that it was obvious what they were doing, and didn’t comment the code. They probably believed that the code was simple enough that a test was unnecessary and it didn’t need to be separated into a well-named method.

I don’t agree with the assumptions I’m led to believe they made, but I do find myself thinking that they must have been smart enough to know what they were doing and that they had a completely valid reason.

In the end, the code works. I’m not going to alter it. I’m not going to rip it out. All that’s going to do is risk reintroducing a bug that was fixed a long time ago. I’d like to add unit tests to cover this, but without know the requirements, a proper unit test would be hard to write. I could extract the code into a separate method, but I’d have to make assumptions about what the intention of the code was so that it could be named properly. Naming the extracted method something similar to TrimIpAddress is going to make future developers believe they know the intent of the code and can test and change it freely.

In the end, the safest thing to do is to leave it in peace and let it keep doing whatever it is it’s doing.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

w

Connecting to %s