The script I have started with is a simple one for registering hosts for DHCP access. Basically, it takes two command line arguments - a fully qualified hostname and a MAC address - and then does some validation, checks that neither address is already in use, normalizes the output to the correct format, constructs a properly formatted host stanza and appends it to the end of our ISC DHCP servers dhcpd.conf configuration file.
I have made improvements to various parts of the code but the changes I am most conflicted about are those I have made to the MAC address normalization function which works reliably and therefore probably isn't a good candidate for refactoring but (to me anyway) looks inelegant - something which I think matters in an elegant language like python.The normalization function takes as inpute a MAC address in one of three (string) formats - unix (00:11:22:33:aa:55), Windows(00-11-22-33-AA-55) and Cisco (0011.2233.aa55) - and the same MAC address in the unix format.
Since I am trying to move towards a more test-driven development approach, I started out by writing a very basic unit test to make sure my new function is at least as reliable as the old function. Here is the code on the unit test (test.py):
Here is my orginal normalization function (oldmac.py):
#!/usr/bin/env python
import unittest
import sys
sys.path.append(".")
import oldmac as mac
#import newmac as mac
class Test(unittest.TestCase):
""" Unit test/s for MAC address normalization function """
normalize_values = (
('00:11:22:33:aa:55', '00:11:22:33:aa:55'),
('0011.2233.aa55', '00:11:22:33:aa:55'),
('00-11-22-33-aa-55', '00:11:22:33:aa:55'),
('00:11:22:33:AA:55', '00:11:22:33:aa:55')
)
def testMacNormalize(self):
""" Normalize MAC addresses to lowercase unix format """
for addr, expected in self.normalize_values:
result = mac.normalize(addr)
self.assertEqual(expected, result)
if __name__ == "__main__":
unittest.main()
There are two things I don't like about this code:
#!/usr/bin/env python
""" Old MAC normalization function """
import re
def normalize(m):
""" Normalize a MAC address to lower case unix style """
m = re.sub("[.:-]", "", m)
m = m.lower()
n = "%s:%s:%s:%s:%s:%s" % (m[0:2], m[2:4], m[4:6], m[6:8], m[8:10], m[10:])
return n
- Use of a regular expression for something as simple as eliminating the delimiters from the string. There must be a simpler way to do this.
- And the bit I find inelegant - the construction of the normalised string, n. It looks ugly :)
So, I set out to rewrite the normalize function more elegantly. Here is the result (newmac.py):
$ ./test.py -v
Normalize MAC addresses to lowercase unix format ... ok
----------------------------------------------------------------------
Ran 1 test in 0.000s
OK
The differences between this version and old version are:
#!/usr/bin/env python
""" New MAC normalization function """
def normalize(addr):
# Determine which delimiter style out input is using
if "." in addr:
delimiter = "."
elif ":" in addr:
delimiter = ":"
elif "-" in addr:
delimiter = "-"
# Eliminate the delimiter
m = addr.replace(delimiter, "")
m = m.lower()
# Normalize
n= ":".join(["%s%s" % (m[i], m[i+1]) for i in range(0,12,2)])
return n
- I replaced the regular expression with a simple string.replace. Why use a (something-big) when a (something-small) will do.
- I replaced the normalization expression with does the same thing but using a list comprehension.
So, what do you think? An improvement or not?
$./test.py -v
Normalize MAC addresses to lowercase unix format ... ok
----------------------------------------------------------------------
Ran 1 test in 0.000s
OK
Have I over-engineered the normalization code by replacing something static, simple and fast with something more complex, less readable and probably slower?
Note: I have just presented one aspect of the refactoring on this script - the MAC address normalization. This is the part I am most conflicted about because the old code worked fine but the new code looks "cooler" :) . The other improvements such as the more robust validation I have left out (perhaps for another blog post).
Comments