• Advertisement
  • entries
    146
  • comments
    436
  • views
    198327

Refactoring, Part 1

Sign in to follow this  

179 views

More often than not, you won't have the luxury of writing code from scratch. Instead you will have inherited a code base. Perhaps you have even been hired on to make changes to an existing application. For the purposes of our discussion, we will assume the latter.

One of the biggest problems with inheriting a previous project is that the code is rarely very well written and, more often than not, contains many of the signs of hasty coding (such as direct copy and pastes). This is a fairly significant problem in the software development industry. Not only does it easily allow bugs to migrate to other portions of the code base, but it also makes extending, or even just plain understanding, the code very difficult.

Typically, along with the code, you will receive a series of requirements that the customer would like implemented. Often implementing the features, without refactoring, will require jumping through many hoops, and may introduce new bugs unknowingly. In fact, modifying the code base at all should be backed up by a robust set of tests, written to both verify the current functionality of the code, and to make sure that all further changes either keep the same exact functionality or fix problems that the tests exposed with the existing code.

So, for the next few episodes of this journal, I'm going to focus on refactoring one particular piece of code (shown in listing 1). The class is part of a framework used by an application to aid network administrators in building and maintaining access control lists on Cisco Pix firewalls. The rules are defined in a fairly straightforward manner, you indicate if the rule is a permit or deny rule. Then you specify the protocol that the rule will match. You then specify a source address and mask pair, this gives the range of source addresses that this rule will match. A destination address and mask pair is also specified. Finally you can optionally provide a port comparison method, and a port.

A few notes: If the protocol contains other protocols, like IP contains TCP and UDP, then it should also apply to those child protocols. Also, just because a particular protocol requires a port for a client to use it doesn't mean that the rule requires a port. For instance you can specify a rule to permit or deny TCP without specifying a port. In this case it will either permit or deny the protocol as a whole.

The client would like this code altered to allow them to easily specify new protocols, along with the supported port ranges of those protocols. All of the new protocols will be IP based, but they may not be direct children of the IP protocol. They also would like the ability to add new comparison methods for matching ports. All of the code should now validate that the ports are within the range allowed by the protocol. If a protocol doesn't allow ports, it should throw an exception. You may alter the public interface of the class in any way required in order to meet these requirements, you may also introduce new classes to aid you in meeting the requirements of the customer.

If you do decide to refactor this, please submit it as a comment here, or start a thread in the .Net forum.

Quote:
Listing 1
public enum Protocol {
Tcp,
Udp,
Ip
}

public enum PortComparison {
GreaterThan,
LessThan,
EqualTo,
None
}

public class AccessListEntry {
public AccessListEntry(bool allowed,
Protocol protocol,
IPAddress sourceAddress,
IPAddress sourceMask,
IPAddress destinationAddress,
IPAddress destinationMask) {
this.allowed = allowed;
this.protocol = protocol;
this.sourceAddress = sourceAddress;
this.sourceMask = sourceMask;
this.destinationAddress = destinationAddress;
this.destinationMask = destinationMask;
this.comparison = PortComparison.None;
this.port = 0;
}

public AccessListEntry(bool allowed,
Protocol protocol,
IPAddress sourceAddress,
IPAddress sourceMask,
IPAddress destinationAddress,
IPAddress destinationMask,
PortComparison comparison,
int port) {
this.allowed = allowed;
this.protocol = protocol;
this.sourceAddress = sourceAddress;
this.sourceMask = sourceMask;
this.destinationAddress = destinationAddress;
this.destinationMask = destinationMask;
this.comparison = comparison;
this.port = port;
}

public bool Match(Protocol protocol, IPAddress sourceIP, IPAddress destinationIP) {
if (protocol == this.protocol || this.protocol == Protocol.Ip) {
byte[] IPBytes = sourceIP.GetAddressBytes();
byte[] AddressBytes = sourceAddress.GetAddressBytes();
byte[] MaskBytes = sourceMask.GetAddressBytes();
for (int i = 0; i < IPBytes.Length; ++i) {
if ((IPBytes & MaskBytes) != (AddressBytes & MaskBytes))
return false;
}

IPBytes = destinationIP.GetAddressBytes();
AddressBytes = destinationAddress.GetAddressBytes();
MaskBytes = destinationMask.GetAddressBytes();
for (int i = 0; i < IPBytes.Length; ++i) {
if ((IPBytes & MaskBytes) != (AddressBytes & MaskBytes))
return false;
}

return true;
}
return false;
}

public bool Match(Protocol protocol, IPAddress sourceIP, IPAddress destinationIP, int port) {
if (protocol == this.protocol || this.protocol == Protocol.Ip) {
byte[] IPBytes = sourceIP.GetAddressBytes();
byte[] AddressBytes = sourceAddress.GetAddressBytes();
byte[] MaskBytes = sourceMask.GetAddressBytes();
for (int i = 0; i < IPBytes.Length; ++i) {
if ((IPBytes & MaskBytes) != (AddressBytes & MaskBytes))
return false;
}

IPBytes = destinationIP.GetAddressBytes();
AddressBytes = destinationAddress.GetAddressBytes();
MaskBytes = destinationMask.GetAddressBytes();
for (int i = 0; i < IPBytes.Length; ++i) {
if ((IPBytes & MaskBytes) != (AddressBytes & MaskBytes))
return false;
}

switch (comparison) {
case PortComparison.GreaterThan:
return port > this.port;
case PortComparison.LessThan:
return port < this.port && port != 0;
case PortComparison.EqualTo:
return this.port == port;
case PortComparison.None:
return true;
}
}
return false;
}

public override string ToString() {
StringBuilder sb = new StringBuilder();
if (allowed) {
sb.Append("permit ");
} else {
sb.Append("deny ");
}

sb.Append(protocol.ToString()).Append(" ");
sb.Append(sourceAddress).Append(" ").Append(sourceMask).Append(" ");
sb.Append(destinationAddress).Append(" ").Append(destinationMask);

switch (comparison) {
case PortComparison.EqualTo:
sb.Append(" eq ").Append(port);
break;
case PortComparison.GreaterThan:
sb.Append(" gt ").Append(port);
break;
case PortComparison.LessThan:
sb.Append(" lt ").Append(port);
break;
case PortComparison.None:
break;
}

return sb.ToString();
}

public bool IsAllowed { get { return allowed; } }

private bool allowed;
private Protocol protocol;
private IPAddress sourceAddress;
private IPAddress sourceMask;
private IPAddress destinationAddress;
private IPAddress destinationMask;
private PortComparison comparison;
private int port;
}
Sign in to follow this  


1 Comment


Recommended Comments

Here's a rough attempt, even though I don't actually know C# :P


// TODO: make a class for these, so they'll print an actual name at runtime.
public enum Protocol {
Tcp,
Udp,
Ip
}

public class ProtocolFilter {
private Protocol prototype;
private ProtocolFilter parentProtocol;
public bool filter(Protocol candidate) {
if (candidate == prototype) return true;
if (parentProtocol == null) return false;
return parentProtocol.filter(candidate);
}
// Non-PortProtocolFilters don't care about the port, so they ignore it.
public bool filter(Protocol candidate, int port) {
return filter(candidate);
}
public StringBuilder addName(StringBuilder sb) {
return sb.Append(" ").Append(prototype.toString());
}
public StringBuilder addPortInfo(StringBuilder sb) {
return sb;
}
public ProtocolFilter(Protocol prototype) {
this.prototype = prototype;
parentProtocol = null;
}
public ProtocolFilter(Protocol prototype, ProtocolFilter parentProtocol) {
this.prototype = prototype;
this.parentProtocol = parentProtocol;
}
}

public class PortProtocolFilter: public ProtocolFilter {
private PortComparator portChecker;
// A protocol filter which expects a port number will always reject a
// connection via a protocol that doesn't specify a port.
public bool filter(Protocol candidate) { return false; }
public bool filter(Protocol candidate, int port) {
return super.filter(candidate) && portChecker.filter(port);
}
public override StringBuilder addPortInfo(StringBuilder sb) {
return portChecker.addInfo(sb);
}
public PortProtocolFilter(Protocol prototype, PortComparator pc) {
this.prototype = prototype;
parentProtocol = null;
portChecker = pc;
}
public PortProtocolFilter(Protocol prototype, ProtocolFilter parentProtocol,
PortComparator pc) {
this.prototype = prototype;
this.parentProtocol = parentProtocol;
portChecker = pc;
}
}

// Some sample protocol filters. Don't shoot me if they're not technically
// accurate.
ProtocolFilter IpFilter = new ProtocolFilter(Ip);
ProtocolFilter TcpFilter = new ProtocolFilter(Tcp, IpFilter);
ProtocolFilter UdpFilter = new ProtocolFilter(Udp, IpFilter);
ProtocolFilter TelnetFilter = new PortProtocolFilter(Udp, IpFilter, new EqualTo(23));
ProtocolFilter HttpFilter = new PortProtocolFilter(Tcp, IpFilter, new EqualTo(80));

public class IPAddressFilter {
private IPAddress addr;
private IPAddress mask;
public bool filter(IPAddress candidate) {
byte[] IPBytes = candidate.GetAddressBytes();
byte[] AddressBytes = addr.GetAddressBytes();
byte[] MaskBytes = mask.GetAddressBytes();
for (int i = 0; i < IPBytes.Length; ++i) {
if ((IPBytes[i] & MaskBytes[i]) != (AddressBytes[i] & MaskBytes[i]))
return false;
}
return true;
}
public IPAddressFilter(IPAddress addr, IPAddress mask) {
this.addr = addr; this.mask = mask;
}
public StringBuilder addInfo(StringBuilder sb) {
return sb.Append(" ").Append(addr).Append(" ").Append(mask);
}
}

// Todo: use delegates instead, or something?
public class PortComparator {
int comparisonPort;
virtual public bool filter(int candidatePort);
public PortComparator(int port) { comparisonPort = port; }
virtual public StringBuilder addInfo(StringBuilder sb);
}

public class GreaterThan : public PortComparator {
public bool filter(int candidatePort) {
return candidatePort > comparisonPort;
}
public StringBuilder addInfo(StringBuilder sb) {
return sb.Append(" gt ").append(comparisonPort);
}
}

public class LessThan : public PortComparator {
public bool filter(int candidatePort) {
return candidatePort < comparisonPort;

Share this comment


Link to comment

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

  • Advertisement