In this post I’ll introduce an example in refactoring. In my last post I wrote about ideas for improving the quality of code. One of the points was that better code would reduce the likelihood of bugs, and hence minimise the need for testing. Also, I proposed ideas for defining user stories and scenarios for having better understanding of what the code actually should do. Then, I’d like to create automated tests before actually creating the code… or refactoring existing code.
In another post I put a simple function together to extract records from an incoming packet. When I looked back at the code, I felt there was a lot of room for improvement. So let’s use this as an example for refactoring. First, here is the original code:
def slicePacket(packet): recordList =  while len(packet) > 0: packetLen = int(packet) recordList.append(packet[1:packetLen + 1]) packet = packet[packetLen + 1:] return recordList
The code is short and simple. What could possibly go wrong? When I simplified it a bit by taking out the binary and decimal conversion (to make it a clearer example), I immediately introduced a bug there.
Problems with the existing function
A few things are obviously wrong here:
- No requirements. What is this function supposed to do, and how?
- No test cases. How do we test this code, in case we have to change something? How can we make sure that we’re not going to introduce more defects?
- No comments. If a developer looks at the code, we can only hope that (s)he grasps what the function is about. The codes does not contain any comments that explain what the function shall do or how it works.
- No error handling. What if the incoming packets are incorrect somehow? Would the program crash? Could a hacker use this function to do evil things?
- Variables are mutated. The recordList array starts empty and new records are added. The packet and packetLen variables are modified all the time as well.
Requirements: Feature and User Story
First, let’s describe what we want the function to do. The function is actually part of something that we could call a “Feature:”
Feature: Process records from incoming Z21 packets.
For this feature, we would need three user stories:
- Connect to the command station and listen to packets
- Extract all records from an incoming packet
- Dispatch extracted records
When applying the IN ORDER TO/AS/I WANT format on user story 2, we’ll get something like:
IN ORDER TO process each Z21 record separately
AS a Z21 client
I WANT to extract an array of records from an incoming packet, where the packet is a string of messages preceded by the length of each message.
Now, things become a lot clearer when we will add a number of scenarios. I describe all scenarios that I could think of that should cover all input/output combinations. I’ll use the GIVEN/WHEN/THEN format to describe the scenarios:
GIVEN an incoming Z21 packet
Scenario 1: one record with a length of 3
WHEN the packet contains “3abc”
THEN the resulting array shall be [“abc”]
Scenario 2: one record with a length of 5
WHEN the packet contains “5abcde”
THEN the resulting array shall be [“abcde”]
Scenario 3: two records
WHEN the packet contains “3abc5abcde”
THEN the resulting array shall be [“abc”,”abcde”]
Scenario 4: no records
WHEN the packet is empty
THEN the resulting array shall be 
Scenario 5: incorrect record (not implemented in the original code)
WHEN the packet contains “abcde” (first character cannot be interpreted as a number)
THEN the resulting array shall be [“error”]
Scenario 6: record too short (not implemented in the original code)
WHEN the packet contains “5abcd” (length of record less than 5)
THEN the resulting array shall be [“error”]
Writing automated tests before the code
We can translate the scenarios above directly to tests. We want to have these tests automated since we’re going to execute them each time we change something in the function code. With each change we want to make sure that we won’t accidentally introduce a bug.
The Test Driven Development (TDD) approach suggests to:
- Write a test and execute it and watch it fail (which we expect).
- Enhance the function so that the test should pass
- Execute the new and all existing tests and verify that they all pass. Then you are sure you didn’t break anything.
- Now if the resulting code is ugly, refactor it. After each refactor step, repeat the tests to ensure that all still work.
I created the following automated test function for the main function.
def extractRecordsTest(): return extractRecords("3abc") == ["abc"] \ and extractRecords("5abcde") == ["abcde"] \ and extractRecords("3abc5abcde") == ["abc", "abcde"] \ and extractRecords("") ==  \ and extractRecords("5abc") == ["error"] \ and extractRecords("abcde") == ["error"]
I built up this list of test cases in parallel with implementing the new “extractRecords” function. (This function shall replace the original “slicePacket” function.)
The refactored function
This is the new extractRecords function that I implemented. With comments, handling of errors and using recursion rather than iteration. Using recursion, more common in functional programming, I avoided having to change the values of variables. This is the new function:
def extractRecords(packet): if packet == "": # end of list, no more records. Natural end of recursion return  if not packet.isnumeric(): # first character does not signify the length return ["error"] recordLength = int(packet) if recordLength > len(packet[1:]): # actual record is too short return ["error"] # one or more records to return record = packet[1:recordLength + 1] restOfPacket = packet[recordLength + 1:] return [record] + extractRecords(restOfPacket)
Does it work?
Whenever I enter:
at the IDLE command line, I should get the response:
I can also add an ad-hoc test myself at the IDLE command line, e.g. by entering:
The result shall then be:
['ab', 'cdef', 'ghijkl']
I think that this new version is much better than the original one. Having clear requirements and automated test cases help. But I also think that the code is clearer and more robust. One could optimise this further, I’m sure. The performance of this solution may not be the best, and that may also be the case for memory consumption. However, in practice I expect only one record to arrive per packet and maybe 2 at most. Performance and memory are not really critical aspects for this specific solution.
Update: a functional version of extractRecords
To be honest, I was happy with the extractRecords function until I added the IF statements for the error conditions. The general format for IF and IF/ELSE in most languages is similar to this:
It looks ugly and it is imperative. After all, statements are used for changing state. As we said before, this is a source for bugs.
Many languages have an alternative in the so-called ternary operation. In C-like languages the format of this is like:
<condition> ? <true_expression> : <false_expression>
Confusingly, in Python this operator is also expressed as IF/ELSE, in the following format:
<true_expression> if <condition> else <false expression>
Expressions do not change the state of a program since there are no statements. If the condition and the expressions themselves are correct, there is no risk for wreaking havoc elsewhere in the program.
I refactored the extractRecords function further as below:
def extractRecords(packet): return \ [ ] if packet == "" else \ [ "error" ] if not packet.isnumeric() else \ [ "error" ] if len(packet) <= int(packet) else \ [ packet[1:int(packet) + 1] ] + \ extractRecords(packet[int(packet) + 1:])
I chained the ternary operators to express all outcomes I needed depending on the various conditions. The automated tests ensure that the conditions and expressions are correct. And there cannot be any side effects since there are no statements besides the obvious return statement.
In this function there is no place for variables. This makes the function truly stateless, but results in that int(packet) had to be evaluated more than once. The final expression that includes the recursion step may be less obvious. On the other hand, the code is much more concise. Adding comments would be an option but it would break the flow and one can wonder if that would be necessary.
What do you think? Is this code better? Is it more readable? Would it be faster or slower? What would be the advantages and disadvantages of this code compared to the imperative version above?