Wondering?
It is a fundamental premise of my work that we can (nearly?) always refactor from a design that no longer serves to one that is better. Can I refactor from my previous OK Collator design to yesterday’s most excellent one? Let’s find out.
If we can refactor in small steps from one design to a desired better one, we can avoid stalling the development pipeline while we work on some big internal “technical debt” work. We can avoid asking for permission to refactor, because refactoring will be made up of small steps that take minutes rather than days or weeks. We can keep the code base alive and our flow of valuable work more smooth and rapid. We can avoid a long branch for a refactoring, followed by a very painful integration.
In short, refactoring in small steps is really quite good. So we’ll try it here.
The Idea
One day, someone shows up in the morning and says:
You know, I think all the work that Collator and Sequencer do could be done by a dictionary and a high-water mark. Python dictionaries are ordered, so the sequence would just drop out.
They draw a picture on the whiteboard:
Again, Python dictionaries keep the keys in the order first presented, just what we need. The red line is a high water mark in the dictionary keys. Above that mark things are new, below they are old. We reset the values to ‘Unrun’ on every ‘with’. That’s all there is to it!
The team are convinced. Pretty easily: they are very bright, they are all me, and anyway it’s a brilliant idea. So we decide to refactor from what we have to this new design, in small steps.
We begin with a review of the top-level class, Collator.
class Collator:
def __init__(self):
self.outcomes = dict()
self.sequencer = Sequencer()
def __enter__(self) -> Self:
self._testing_begin()
return self
def __exit__(self, *args):
pass
def add(self, name: str, outcome: str):
self.outcomes[name] = outcome
self.sequencer.add_name(name)
def results(self) -> Generator[Result, None, None]:
return (self._result_for(aged_name.name, aged_name.is_new)
for aged_name in self.sequencer.aged_names())
def _result_for(self, name: str, is_new: bool)-> Result:
return Result(name, self._outcome_for(name), is_new)
def _outcome_for(self, name: str) -> str:
return self.outcomes.get(name, "Unrun")
# noinspection PyProtectedMember
def _testing_begin(self):
self.sequencer._testing_begin()
self.outcomes = dict()
Right away we notice that testing method. It’s there because our design started by using a call to begin to prime the Collator for a new batch. Our production code uses the with but we never retrofitted our tests to use with. We decide to do that now, if it’s as easy as we expect. Here’s an example:
def test_add_name_provides_result(self):
collator = Collator()
collator._testing_begin()
collator.add(name='TestFoo', outcome='Fail')
result = collator._result_for('TestFoo', True)
assert result.name == 'TestFoo'
assert result.outcome == 'Fail'
assert result.is_new is True
We try the obvious change:
def test_add_name_provides_result(self):
collator = Collator()
with collator:
collator.add(name='TestFoo', outcome='Fail')
result = collator._result_for('TestFoo', True)
assert result.name == 'TestFoo'
assert result.outcome == 'Fail'
assert result.is_new is True
The test still passes, we are not surprised.
I’m going to try to commit each and every time things go green. I’ll do that for two reasons. First of all, I think it’s a really good practice to keep my steps small, and that I will do better if I commit on every green unless I have a good reason not to.
Second, for this exercise, which could take place over a long period of the team’s time, as we work mostly on new features, Id like to get a sense of how long the refactoring could be stretched out without ever stalling delivery or needing to branch the repo.
Commit: changed a test to use with.
Along the way (as it happens, the very next test we look at) we see behavior that we probably want to change:
def test_missing_name_provides_unrun_result(self):
collator = Collator()
with collator:
result = collator._result_for('TestBar', True)
assert result.name == 'TestBar'
assert result.outcome == 'Unrun'
assert result.is_new is True
Our current implementation will return ‘Unrun’ for a test it has never heard of, because we’re defaulting the result. That might actually be a less than desirable notion. It might be better to return ‘Unknown’. We may need figure out whether we want to do something about that. Anyway, we leave the test working as is. If we change that behavior later we’ll deal with it.
Commit again. And again. And again, over time, until, soon enough, we’ve converted all the tests to use with. The only user of _testing_begin is __enter__ so we inline and remove it.
class Collator:
def __enter__(self) -> Self:
self.sequencer._testing_begin()
self.outcomes = dict()
return self
Commit: refactoring Collator.
We turn our attention to Sequencer:
class Sequencer:
def __init__(self):
self.new_names = []
self.known_names = []
def add_name(self, name: str): # 8 usages
if name not in self.known_names and name not in self.new_names:
self.new_names.append(name)
def aged_names(self) -> Generator[AgedName, None, None]: # 6 usages
yield from self._yield_known_names()
yield from self._yield_and_age_new_names()
self.new_names = []
def _yield_known_names(self) -> Generator[AgedName, None, None]:
for name in self.known_names:
yield AgedName(name=name, is_new=False)
def _yield_and_age_new_names(self) -> Generator[AgedName, None, None]:
for name in self.new_names:
self.known_names.append(name)
yield AgedName(name=name, is_new=True)
def _testing_begin(self): # 7 usages
self.known_names.extend(self.new_names)
self.new_names = []
We see that there is substantial direct testing for Sequencer. It may be undesirable to try to evolve it. Instead, we might try a “strangler vine” approach, the term invented (I believe) by Michael Feathers in Working Effectively with Legacy Code and as documented by Martin Fowler, probably in Refactoring. The idea of the pattern is to implement our new capability bit by bit, relying on the existing code (Sequencer) where we must, until finally it is no longer needed.
Looking at Sequencer, we see that all it “really” does is produce a list of AgedName instances, with name and is_new flag.
We ought to be able to do that in Collator, which is maintaining a dictionary of names anyway.
However, Collator creates a new dictionary on each execution:
class Collator:
def __init__(self):
self.outcomes = dict()
self.sequencer = Sequencer()
def __enter__(self) -> Self:
self.sequencer._testing_begin()
self.outcomes = dict()
return self
We want different behavior now. What if we just don’t create that new dictionary in __enter__. Wouldn’t everything just continue to work? And if not, we’d learn something important about this little project.
With that removed, two tests fail, both the same way, returning a result when ‘Unrun’ was expected. We realize that we need to do what our new idea said: set all the existing entries to ‘Unrun’ on entry:
def __enter__(self) -> Self:
self.sequencer._testing_begin()
for name in self.outcomes.keys():
self.outcomes[name] = 'Unrun'
return self
We are green. Commit: refactoring Collator. Set all existing outcomes to ‘Unrun’ on ‘with’
We find ourselves with this code:
def _result_for(self, name: str, is_new: bool)-> Result:
return Result(name, self._outcome_for(name), is_new)
def _outcome_for(self, name: str) -> str:
return self.outcomes.get(name, "Unrun")
This might be the time to return ‘Unknown’. We decide not to change the behavior in mid-refactoring, and we make a card to think about this default later. Or set a reminder to trigger every Monday until we stop it. Or whatever we use to remember things for later.
We only have one reference to sequencer where we fetch anything:
def results(self) -> Generator[Result, None, None]:
return (self._result_for(aged_name.name, aged_name.is_new)
for aged_name in self.sequencer.aged_names())
We pull out a method:
def results(self) -> Generator[Result, None, None]:
return (self._result_for(aged_name.name, aged_name.is_new)
for aged_name in self.aged_names())
def aged_names(self):
return self.sequencer.aged_names()
Green. Commit: Isolate aged_names method for later replacement. Once Collator is ready to produce aged names on its own, we will just plug the code into that method, cutting off sequencer.
We need to maintain our own high water mark:
class Collator:
def __init__(self):
self.outcomes = dict()
self.sequencer = Sequencer()
self.high_water = 0
def __enter__(self) -> Self:
self.sequencer._testing_begin()
for name in self.outcomes.keys():
self.outcomes[name] = 'Unrun'
self.high_water = len(self.outcomes)
return self
We need a test for that, although we’re sure it’s right. Let’s enhance an existing test:
def test_story_test(self):
collator = Collator()
with collator:
assert collator.high_water == 0
collator.add(name='TestFoo', outcome='Pass')
collator.add(name='TestBar', outcome='Fail')
initial_result = list(collator.results())
self.check(initial_result, 0,
'TestFoo', 'Pass', True)
self.check(initial_result, 1,
'TestBar', 'Fail', True)
with collator:
assert collator.high_water == 2
collator.add(name='TestBaz', outcome='Pass')
collator.add(name='TestBar', outcome='Pass')
second_result = list(collator.results())
self.check(second_result, 0,
'TestFoo', 'Unrun', False)
self.check(second_result, 1,
'TestBar', 'Pass', False)
self.check(second_result, 2,
'TestBaz', 'Pass', True)
Green. Commit: keeping track of high water in Collator.
We may be ready to get the AgedNames directly inside Collator now. Let’s write a little test for that feature.
def test_collator_aged_names(self):
collator = Collator()
with collator:
collator.add(name='TestFoo', outcome='Pass')
collator.add(name='TestBar', outcome='Fail')
with collator:
collator.add(name='TestBaz', outcome='Pass')
aged = collator.get_aged_names()
assert len(aged) == 3
assert [a.name for a in aged] == ['TestFoo', 'TestBar', 'TestBaz']
assert [a.is_new for a in aged] == [False, False, True]
I think that’s right. We add a new method name, because we don’t want to break everything until this test works. And then still not break everything.
def get_aged_names(self):
return [AgedName(k, i>= self.high_water)
for i, (k, v) in enumerate(self.outcomes.items())]
Commit: Can compute AgedNames inside Collator.
We note that the aged_names method in Sequencer is a generator. Change our test:
def test_collator_aged_names(self):
collator = Collator()
with collator:
collator.add(name='TestFoo', outcome='Pass')
collator.add(name='TestBar', outcome='Fail')
with collator:
collator.add(name='TestBaz', outcome='Pass')
aged = list(collator.get_aged_names())
assert len(aged) == 3
assert [a.name for a in aged] == ['TestFoo', 'TestBar', 'TestBaz']
assert [a.is_new for a in aged] == [False, False, True]
And our get:
def get_aged_names(self) -> Generator[AgedName, None, None]:
return (AgedName(k, i>= self.high_water)
for i, (k, v) in enumerate(self.outcomes.items()))
Green. Commit: convert get_aged_names to generator.
We think we can replace our call to aged_names in Sequencer now. We’ll remove that forwarding method and then rename the get we just wrote:
def aged_names(self) -> Generator[AgedName, None, None]:
return (AgedName(k, i>= self.high_water)
for i, (k, v) in enumerate(self.outcomes.items()))
Green. Commit: sequencer no longer used for Aged_Names.
We can now remove all the references to Sequence from the class. There are three.
class Collator:
def __init__(self):
self.outcomes = dict()
self.sequencer = Sequencer()
self.high_water = 0
def __enter__(self) -> Self:
self.sequencer._testing_begin()
for name in self.outcomes.keys():
self.outcomes[name] = 'Unrun'
self.high_water = len(self.outcomes)
return self
def add(self, name: str, outcome: str):
self.outcomes[name] = outcome
self.sequencer.add_name(name)
We could do these one at a time to get three commits. That would be too silly even for me. I do remove them one at a time and watch the tests stay green.
Commit: Collator no longer uses Sequencer. Sequencer and its tests can be removed. We’ll save that for the thrilling conclusion.
This could be inlined:
def results(self) -> Generator[Result, None, None]:
return (self._result_for(aged_name.name, aged_name.is_new)
for aged_name in self.aged_names())
def _result_for(self, name: str, is_new: bool)-> Result:
return Result(name, self._outcome_for(name), is_new)
But there are two tests that test this method:
def test_add_name_provides_result(self):
collator = Collator()
with collator:
collator.add(name='TestFoo', outcome='Fail')
result = collator._result_for('TestFoo', True)
assert result.name == 'TestFoo'
assert result.outcome == 'Fail'
assert result.is_new is True
def test_missing_name_provides_unrun_result(self):
collator = Collator()
with collator:
result = collator._result_for('TestBar', True)
assert result.name == 'TestBar'
assert result.outcome == 'Unrun'
assert result.is_new is True
We declare these to be redundant. They were scaffolding, and our final large tests cover them. Remove.
def results(self) -> Generator[Result, None, None]:
return (Result(a_n.name, self._outcome_for(a_n.name), a_n.is_new)
for a_n in self.aged_names())
Green. Commit: inline method.
We should be able to combine these two methods and stop using the Aged_Name intermediary object:
def results(self) -> Generator[Result, None, None]:
return (Result(a_n.name, self._outcome_for(a_n.name), a_n.is_new)
for a_n in self.aged_names())
def aged_names(self) -> Generator[AgedName, None, None]:
return (AgedName(k, i>= self.high_water)
for i, (k, v) in enumerate(self.outcomes.items()))
I was going to edit the second method to return the result, remove the first and rename the second. Curiously, it runs in this form:
def results(self) -> Generator[Result, None, None]:
return (Result(a_n.name, self._outcome_for(a_n.name), a_n.is_new)
for a_n in self.aged_names())
def aged_names(self) -> Generator[Result, None, None]:
return (Result(k, v, i>= self.high_water)
for i, (k, v) in enumerate(self.outcomes.items()))
The reason is that both AgedName and Result respond to the name and is_new. Anyway continue with my plan, though I could commit now.
In this form:
def results(self) -> Generator[Result, None, None]:
return (Result(k, v, i>= self.high_water)
for i, (k, v) in enumerate(self.outcomes.items()))
Our scaffolding test for AgedNames fails, because we don’t have the method any more:
def test_collator_aged_names(self):
collator = Collator()
with collator:
collator.add(name='TestFoo', outcome='Pass')
collator.add(name='TestBar', outcome='Fail')
with collator:
collator.add(name='TestBaz', outcome='Pass')
aged = list(collator.aged_names())
assert len(aged) == 3
assert [a.name for a in aged] == ['TestFoo', 'TestBar', 'TestBaz']
assert [a.is_new for a in aged] == [False, False, True]
I think if we change that to call for results it will run.
def test_collator_aged_names(self):
collator = Collator()
with collator:
collator.add(name='TestFoo', outcome='Pass')
collator.add(name='TestBar', outcome='Fail')
with collator:
collator.add(name='TestBaz', outcome='Pass')
aged = list(collator.results())
assert len(aged) == 3
assert [a.name for a in aged] == ['TestFoo', 'TestBar', 'TestBaz']
assert [a.is_new for a in aged] == [False, False, True]
It does. I feel better about that than about removing it. Commit: no longer using AgedNames class in Collator.
Somewhere along the way the _outcomes_for method became unused. Remove it and commit.
Here is Collator:
class Collator:
def __init__(self):
self.outcomes = dict()
self.high_water = 0
def __enter__(self) -> Self:
for name in self.outcomes.keys():
self.outcomes[name] = 'Unrun'
self.high_water = len(self.outcomes)
return self
def __exit__(self, *args):
pass
def add(self, name: str, outcome: str):
self.outcomes[name] = outcome
def results(self) -> Generator[Result, None, None]:
return (Result(k, v, i>= self.high_water)
for i, (k, v) in enumerate(self.outcomes.items()))
AgedName class is used in a half dozen tests, Sequencer in five. Remove all the sequencer tests and the class. We could do this with six commits if we wanted to.
Commit: remove Sequencer and its tests AgedName has no references. Safe Delete.
Summary
We have successfully refactored from the old Collator+Sequencer design to the new high-water scheme. Along the way we removed a few redundant tests and adjusted a few others. We never broke anything for more than a moment and the job was done in—let me count them—11 commits, each one leaving everything green. We could have done those a day apart, a week apart, whatever.
Of course this proves nothing. These four related classes, Collator, Sequencer, AgedName, and Result were all small, with very limited responsibilities in each class, all nicely separated out. This tells us nothing about the big intertwined classes in “real code”.
Unless … unless … maybe it’s telling us that small classes with limited responsibilities are easy to change when we need to. Maybe … just maybe … we’d do well to work with small classes instead of those multiple hundred line ones we seem to wind up with.
OK. I’m really nearly sure that we’re done with the Collator question now … unless I decide to draw some lessons. No promises. I have the luxury of working on and writing about whatever interests me.
See you next time!