• entries
146
436
• views
198530

# Refactoring, Part 2

177 views

Alright, well, this entry will continue from where the last one left off.

Now that you've had a few days to look over the code, and get a feel for it, lets start refactorings it. Now, one of the big things about refactoring is that there are so many different ways of refactoring code. In fact, one could almost say: No two ways of refactoring will have the results, except in that the code will become simpler. So, I will attempt to point out some of the alternatives as I writes this, not all of them though.

public AccessListEntry(bool allowed, Protocol protocol,	IPAddress sourceAddress, IPAddress sourceMask,	IPAddress destinationAddress, IPAddress destinationMask) :	this(allowed, protocol, sourceAddress, sourceMask,	destinationAddress, destinationMask, PortComparison.None, 0){ }
Lets start by looking at the constructors. Viewing them just as they are written one can immediately see that there is a rather large amount of code duplication. In fact it would almost appear that the programmer just copied and pasted into the other (which in fact he did cough cough). Now, we want to remove this obvious duplication of code. There are really two ways you can do this, the first is to refactor the code out into it's own method, which both constructors call. The second way is to refactor it such that one constructor calls the other. It doesn't really matter which you chose, although for people not familiar with the language, the former would certainly be easier to understand. However, I'm going to use the latter. Be sure to run your tests after every modification. As you can see above, the new code is much shorter, although not as easy to read as using a separate method would be.

public AccessListEntry(bool allowed, Protocol protocol,	IPAddress sourceAddress, IPAddress sourceMask,	IPAddress destinationAddress, IPAddress destinationMask,	PortComparison comparison, int port) {	if (sourceAddress == null)		throw new ArgumentNullException("sourceAddress");	if (sourceMask == null)		throw new ArgumentNullException("sourceMask");	if (destinationAddress == null)		throw new ArgumentNullException("destinationAddress");	if (destinationMask == null)		throw new ArgumentNullException("destinationMask");	if (comparison != PortComparison.None) {		if (protocol == Protocol.Ip)			throw new ArgumentException("IP does not support ports.");		if (port < 1 || port > 65535)			throw new IndexOutOfRangeException(				"Ports must be in the range [1,65535]");	}
Looking at the second constructor we notice that it has no error checking, and viewing our customer requirements, we need to add such checks. Looking over the code we see that I've added some checks to make sure that the passed in addresses and masks are not null. Then I check if the comparison method is something other than PortComparison.None. If it is then I check that the protocol supports ports (ie, isn't IP in this case). Then we do some basic range checking. Again, be sure to run your tests frequently as you make these changes. In fact, right about now would be a good time to write add some tests that verify that your checks work.
One of the things you should notice is that these refactorings have opened the way for us to add our own range checking for ports based on protocols. It's also put into place the framework to allow us to check if a protocol even supports ports.

public bool Match(Protocol protocol, IPAddress sourceIP, IPAddress destinationIP) {	return Match(protocol, sourceIP, destinationIP, 0);}
Moving on we get to the Match() method. Again, looking over these we see that the first is basically a copy and paste of the second, minus a few features. However, before we start refactoring, I thought I would point out a minor bug in this code. Specifically, if the access list was created and a port/port comparison was specified, then this function will ignore that. Hence you may get a passing rule, even when it should fail. Our refactoring is ridiculously simple though, we just have the first Match() method call the second, passing a port of 0. This not only fixes the bug (as the second does check for port matching), but it also halves our code.

private bool MatchAddress(IPAddress ip, IPAddress network, IPAddress mask) {	byte[] ipBytes = ip.GetAddressBytes();	byte[] addressBytes = network.GetAddressBytes();	byte[] maskBytes = mask.GetAddressBytes();	for (int i = 0; i < ipBytes.Length; ++i) {		if ((ipBytes & maskBytes) != (addressBytes & maskBytes))			return false;	}	return true;}
Now, looking at the second Match() method we see that there appears to be some more duplicate code. Specifically the code that deals with IP matching. So, pull out a new method, and call it MatchAddress(). Then replace the original code which dealt with matching the addresses with calls to this function. We also add some checks to the Match() method to ensure that the source IP and destination IP are not null.

public bool Match(Protocol protocol, IPAddress sourceIP, IPAddress destinationIP, int port) {	if (sourceIP == null)		throw new ArgumentNullException("sourceIP");	if (destinationIP == null)		throw new ArgumentNullException("destinationIP");	if (protocol == this.protocol || this.protocol == Protocol.Ip) {		if (!MatchAddress(sourceIP, sourceAddress, sourceMask))			return false;		if (!MatchAddress(destinationIP, destinationAddress, destinationMask))			return false;		return ComparePort(port);	}	return false;}
The final refactoring I would do to this method would be to pull the port comparison code out to it's own method. Having completed these refactorings, you should once again make sure all of your tests pass.

A test that tests for passing, and fails is often less useful than a test that tests for failure and passes. As was the case for the bug in the initial match test. But tests that only test for failure aren't always A Good Thing&tm; Instead it is better to test for a mixture of the two. It is also prudent to note that copy and paste programming, while a bad thing to do in production code, is perfectly acceptable in test code. The reasoning is simple: The tests should be short and to the point. So why spend time performing major refactorings on them? Now, that's not to say you shouldn't pull out methods for common operations. As that can simplify writing tests, and also make it easier to find out where a test is failing. But that does suggest that tests don't always have to meet the stringent coding standards of the production code base, although they should be close.

I'm not entirely sure if you're writing this with a focus on .Net or on general C++, but is there a reason you decided to keep the two constructors, and the two functions instead of simply removing the shorter one of each and giving the port argument a default value?

.Net doesn't support default parameters. In fact, most languages don't. It's a rather odd artifact of C++ that I tend to suggest people avoid, mostly for clarity reasons. See, when I'm reading code, and I see a method call, I would rather just be able to read it's name and tell what it does directly. Default parameters break this because you now have hidden values being passed around sometimes that you actually have to look at the method signature to know about. Thus the behavior might not be obvious.

## Create an account

Register a new account