Refactoring III: Tests in Place

It’s time for our third article on refactoring Dizzi’s leg animation code. Here I am, sitting in NCI Kuula, with a few hundred of my closest friends. Fortunately for you, the picture does not show the giant scary three-headed dragon who was just here.  Sometimes it’s really hard to concentrate in Kuula. 

We have the code pretty well under test now. Each function has its own tests and we can be pretty sure that the sequences of settings to the legs are going as planned. There are some things that are not tested in that regard. Can you think of one that might be important? I can think of some, but I’m going to let it slide for now. It may come up later.

Now that the code is under test, we can take a look at it overall to see what we think needs improvement. We’ve already made one change that I like, which is that the setVisible() and setInvisible() functions are used to cover up the more complex and repetitive calls to llSetLinkAlpha(). (We actually built setVisible() and setInvisible() to enable testing, by putting the parameters in a list that we can check.

Looking at the code overall, I look for duplication that can be removed, and for code that does not express very clearly what it is doing. (See my articles on intention.) You can view the code from the previous article, and I’ll quote bits here as I talk about them.

The first duplication I see is the repetitive definition of blocks of five integer variables:

integer LF2       = 9;
integer LF3       = 7;
integer LF4       = 4;
integer LF5       = 8;
integer LF_STAND  = 4;

integer RL2       = 14;
integer RL3       = 10;
integer RL4       = 15;
integer RL5       = 13;
integer RL_STAND  = 10;

integer RF2       = 12;
integer RF3       = 11;
integer RF4       = 17;
integer RF5       = 16;
integer RF_STAND  = 12;

integer RR2       = 21;
integer RR3       = 20;
integer RR4       = 19;
integer RR5       = 18;
integer RR_STAND  = 18;

We see here four groups, one for each leg, and five values, one for each of the four moving positions of the legs, plus one calling out which position is the standing one. Now in some reasonable programming language, this information might be stored in arrays, or even objects.  In LSL we have very few choices. There is this form, which is simple enough in concept and a little bit clunky in practice, or perhaps we could use a list, which will likely look simpler in the definition but require more complex code to use it. It’s tempting to try it.

We see related duplication in the code as well, since code and data always go together. Here are a couple of the corresponding functions:

step3 ()
{
    setInvisible(LF2);
    setVisible  (LF3);

    setInvisible(RL2);
    setVisible  (RL3); 

    setInvisible(RF2);
    setVisible  (RF3);

    setInvisible(RR2);
    setVisible  (RR3);
}

step4 ()
{
    setInvisible(LF3);
    setVisible(LF4);

    setInvisible(RL3);
    setVisible(RL4);

    setInvisible(RF3);
    setVisible(RF4);

    setInvisible(RR3);
    setVisible(RR4);
}

Now these are the two simplest functions. Each one just sets the previous leg off and the current one on.  step5() is similar, but step2() and stand() are more complex:

stand ()
{
    setVisible(LF_STAND);
    setVisible(RL_STAND);
    setVisible(RF_STAND);
    setVisible(RR_STAND);

    setInvisible(LF2);
    setInvisible(LF3);
    setInvisible(LF5);

    setInvisible(RL2);
    setInvisible(RL4);
    setInvisible(RL5);

    setInvisible(RF3);
    setInvisible(RF4);
    setInvisible(RF5);

    setInvisible(RR2);
    setInvisible(RR3);
    setInvisible(RR4);
}

step2 ()
{
    setVisible(LF2);
    setInvisible(LF4); //stand
    setInvisible(LF5);

    setVisible(RL2);
    setInvisible(RL3); //stand
    setInvisible(RL5);

    setVisible(RF2); //stand
    setInvisible(RF5);

    setVisible(RR2);
    setInvisible(RR5); //stand
}

The step2() function needs to turn off both the previous step (step5()) but also the stand position, in case we are just starting out.  And the stand() function doesn’t know at all what position we were in before, so it turns them all off. If all the functions had exactly the same shape, it might be easier to remove their duplication. In addition, we need to be aware of timing concerns. Note that the script does one leg and then another, then another.  Dizzi tells me that if you turn one off before turning on the next, there can be a visible flicker. But some of these go invisible first, and some go visible first. Oddness.

To preserve the function of this code, we need to duplicate what it does. But we may also have discovered a subtle bug, which we will need to fix. Anway, and I’m just guessing here, it looks like we’re better off to do one leg at a time, either off-on or on-off … but not all legs on and then all off or vice versa.

And that’s what our tests do not check. They check to see that the right legs are turned on and off, and also check that the visibles and invisibles are in the right order, but they do not check to be sure that they alternate. Can you think of how we might change or improve the tests to check for that? And should we?

Improve the tests?

When we become aware of some property of the program that could matter, we would like that property to be enforced by out tests. Otherwise, as we try to improve the code, we might break something that is important to us. 

At the same time, we’re here to make the code better, not to test forever. So there’s always a decision to be made about whether to make the tests stronger, or not. I’m always tempted not to add tests. Sometimes I get away with it, and sometimes I get in trouble. Should I do the pure thing here, or risk making a mistake in front of everyone? Hmmm …

I don’t feel like embarrassing myself after saying what I could do to avoid it. (That leaves open some new way of embarrassing myself, so stay tuned.) I think I’ll do theadditional  test.

One reason I’m willing to do the test is that I can think of a fairly easy way to do it. We want to ensure that the actions take place in exactly the same order as they do now. So what if we keep the existing visible and invisible list, and also add another list, called, maybe, “order”, recording the order things are done in. Then we’d just have to clear it at the beginning of each test, update it in the setVisible() and setInvisible() functions, and then check it at the end of each test. Should be easy enough. I’ll add that next.

// JR Testing Starts Here

list visible;
list invisible;
list ordering;
...
testStep2() {
    visible = [];
    invisible = [];
    ordering = [];
    step2();
    assertEqual(visible, [9, 14, 12, 21]);
    assertEqual(invisible, [4, 8, 10, 13, 16, 18]);
    assertEqual(ordering, []);
}

I’ve highlighted the line added in the data, and the two lines added in the testStep2() function.  I also noticed that all the tests begin with the same two lines clearing visible and invisible, and they will soon need to begin with the same three. This is duplication and I was taught to remove it. It’s also test initialization and that is usually done by extracting into a function called setup(). I think since I’m trying to tread lightly, I’ll call it setupTest(). I’ll make that change in just this one test for now:

setupTest() {
    visible = [];
    invisible = [];
    ordering = [];
}

testStep2() {
    setupTest();
    step2();
    assertEqual(visible, [9, 14, 12, 21]);
    assertEqual(invisible, [4, 8, 10, 13, 16, 18]);
    assertEqual(ordering, []);
}

OK. I’ve just changed the one for now, but I will need to change them all when the time comes. One more note: I’ve left the expected value in the ordering test empty. That will ensure a failure, and my plan is to copy the correct value from the error message and paste it in. Normally that would be a no-no, because I’d just be testing that the program does what the program does … but in this case that’s what we’re trying to do. These are characterization tests: we’re just recording what the program actually does so that we can’t accidentally make it do something different later. So just one test run and then I should be green again.

Yeah, right. I was expecting a failure, and I didn’t get it. This is why we always run the test even though we expect it to fail: sometimes it doesn’t. What went wrong? Well, in the setVisible and setInvisible functions, I didn’t save anything into ordering. Duh. Here’s that code:

setInvisible(integer link) {
    llSetLinkAlpha(link, 0, ALL_SIDES);
    invisible += [link];
    ordering += [link];
}

setVisible(integer link) {
    llSetLinkAlpha(link, 1, ALL_SIDES);
    visible += [link];
    ordering += [link];
}

OK, that fails nicely, saying “Lists are not equal, [9, 4, 8, 14, 10, 13, 12, 16, 21, 18] != []“. Notice that the result string is just the two other output lists, interleaved. So we believe the answer and we can plug it into the test to get a green result. I just did that and of course the test runs. Now I’ll do the same thing with the other tests as well. Hold on a moment … that went quite smoothly. Here, for the record, are all the tests, nicely enhanced:

testStep2() {
    setupTest();
    step2();
    assertEqual(visible, [9, 14, 12, 21]);
    assertEqual(invisible, [4, 8, 10, 13, 16, 18]);
    assertEqual(ordering, [9, 4, 8, 14, 10, 13, 12, 16, 21, 18]);
}

testStep3() {
    setupTest();
    step3();
    assertEqual(visible, [7, 10, 11, 20]);
    assertEqual(invisible, [9, 14, 12, 21]);
    assertEqual(ordering, [9, 7, 14, 10, 12, 11, 21, 20]);
}

testStep4() {
    setupTest();
    step4();
    assertEqual(visible, [4, 15, 17, 19]);
    assertEqual(invisible, [7, 10, 11, 20]);
    assertEqual(ordering, [7, 4, 10, 15, 11, 17, 20, 19]);
}

testStep5() {
    setupTest();
    step5();
    assertEqual(visible, [8, 13, 16, 18]);
    assertEqual(invisible, [4, 15, 17, 19]);
    assertEqual(ordering, [4, 8, 15, 13, 17, 16, 19, 18]);
}

testStand() {
    setupTest();
    stand();
    assertEqual(visible, [4,10,12,18]);
    assertEqual(invisible, [9, 7, 8, 14, 15, 13, 11, 17, 16, 21, 20, 19]);
    assertEqual(ordering, [4, 10, 12, 18, 9, 7, 8, 14, 15, 13, 11, 17, 16, 21, 20, 19]);
}

Surprised? I am, a bit …

I had thought that I’d get to making my changes in this article, but it was not to be. It seemed like a very good idea to add these additional characterization tests, especially since I have a good idea of what I want to try next. Let me tell you that now, and then we can see what I actually do, next time.

Looking back at the step2() example, or any of them, we see that what happens is that we process each leg one at a time, first turning on what should be visible, then turning off what should be invisible. So I’m thinking of a new function, kind of like changeVisibility(list on, list off). Then I’d like to go through the two lists together, turning on one item and off one item. That’s what I’d like to do. I can already see that it won’t quite work, because this very step2() function we’re talking about sometimes turns off two links, and sometimes one. It’s going to be more intricate than that, and I’m not sure yet just how to go about it. 

If lists worked like they should, you could insert a list into a list and that list element would then contain a list, just as a list element can contain an integer or a string or a vector. But you can’t! Insert a list into a list and the list is flattened out for your inconvenience. 

I can see a few things to try, none of which are as simple as I’d like. It may turn out that none of them is good enough. I’ll talk about that next time. For now, here are my ideas:

1. Use three lists, on, off, and extraOff. Interleave all three in order. This is my current favorite, though it has a little complexity added by the need to deal with the lists not all matching. In addition, this won’t handle the stand() case where we have to turn off four items, not just one or two.
2. In the off list, allow multiple items to be turned off to be stored in a vector. Thus to turn off item 1 on the first step and 2 and 3 on the next, then just 4, then 5, 6, and 7, we could use the list [1,<2,3,0>,4,<5,6,7>]. I’m sure I could make this work but it seems awfully fancy. Also, I think in one of the steps I actually have to turn four items. Still, I could use a rotation instead of a vector. Still awfully fancy.
3. In the off list, put a count in the list of how many to turn off in the next step. The same list above might turn into [1,1,2,2,3,1,5,3,5,6,7]. I’m really sure that’s right. If you aren’t so sure, then you see why I don’t like that idea: it is really error-prone. 
4. Extract all the extra offs into one list and process it at the end. This would change the order of things, and result in turning some legs off a bit later, sometimes. The characterization tests would make it fairly easy to be sure we had things right … but we would have to change the tests while refactoring. That’s always risky.

So those are my ideas. None of them makes me entirely happy. And none of them may turn out to be better, which you would think might be distressing after all this work. We’ll talk about that if it happens, but here’s what I’m sure about: if I tried any one of those things without these tests, I could make a mistake–probably would make a mistake–and never know about it. I’d rather have my tests and not need them, than make a mistake and not find it.

See you next time!