An Experiment in Refactoring

I’ve been taught to keep code designs clean using a practice called “Refactoring”. For details, check out Martin Fowler’s site, www.refactoring.com. Briefly, refactoring is a process of improving the design of code by making small changes that preserve function while improving one or another design element. My sister Dizzi Sternberg wrote an interesting script for her horse-drawn cart over the last couple of days, and just for fun, I’m going to try to refactor it.

 

The script Dizzi wrote works the legs on the horse that pulls a cart around Lexicolo, one of our tours of our area. If you haven’t checked it out, come take a look. The script makes it look like the horse is walking. The actual horse object has sixteen legs, four positions for each of four legs. The script makes the legs appear to move by making all but one leg at each position transparent. It works quite nicely. Here’s the script as Dizzi wrote it:

integer Root      = 1;

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;

integer running;

integer num;

stand ()
{
    llSetLinkAlpha(LF_STAND, 1, ALL_SIDES);
    llSetLinkAlpha(RL_STAND, 1, ALL_SIDES);
    llSetLinkAlpha(RF_STAND, 1, ALL_SIDES);
    llSetLinkAlpha(RR_STAND, 1, ALL_SIDES);

    llSetLinkAlpha(LF2, 0, ALL_SIDES);
    llSetLinkAlpha(LF3, 0, ALL_SIDES);
    llSetLinkAlpha(LF5, 0, ALL_SIDES);

    llSetLinkAlpha(RL2, 0, ALL_SIDES);
    llSetLinkAlpha(RL4, 0, ALL_SIDES);
    llSetLinkAlpha(RL5, 0, ALL_SIDES);

    llSetLinkAlpha(RF3, 0, ALL_SIDES);
    llSetLinkAlpha(RF4, 0, ALL_SIDES);
    llSetLinkAlpha(RF5, 0, ALL_SIDES);

    llSetLinkAlpha(RR2, 0, ALL_SIDES);
    llSetLinkAlpha(RR3, 0, ALL_SIDES);
    llSetLinkAlpha(RR4, 0, ALL_SIDES);
}

step2 ()
{
    llSetLinkAlpha(LF2, 1, ALL_SIDES);
    llSetLinkAlpha(LF4, 0, ALL_SIDES); //stand
    llSetLinkAlpha(LF5, 0, ALL_SIDES);

    llSetLinkAlpha(RL2, 1, ALL_SIDES);
    llSetLinkAlpha(RL3, 0, ALL_SIDES); //stand
    llSetLinkAlpha(RL5, 0, ALL_SIDES);

    llSetLinkAlpha(RF2, 1, ALL_SIDES); //stand
    llSetLinkAlpha(RF5, 0, ALL_SIDES);

    llSetLinkAlpha(RR2, 1, ALL_SIDES);
    llSetLinkAlpha(RR5, 0, ALL_SIDES); //stand
}

step3 ()
{
    llSetLinkAlpha(LF2, 0, ALL_SIDES);
    llSetLinkAlpha(LF3, 1, ALL_SIDES);

    llSetLinkAlpha(RL2, 0, ALL_SIDES);
    llSetLinkAlpha(RL3, 1, ALL_SIDES); 

    llSetLinkAlpha(RF2, 0, ALL_SIDES);
    llSetLinkAlpha(RF3, 1, ALL_SIDES);

    llSetLinkAlpha(RR2, 0, ALL_SIDES);
    llSetLinkAlpha(RR3, 1, ALL_SIDES);
}

step4 ()
{
    llSetLinkAlpha(LF3, 0, ALL_SIDES);
    llSetLinkAlpha(LF4, 1, ALL_SIDES);

    llSetLinkAlpha(RL3, 0, ALL_SIDES);
    llSetLinkAlpha(RL4, 1, ALL_SIDES);

    llSetLinkAlpha(RF3, 0, ALL_SIDES);
    llSetLinkAlpha(RF4, 1, ALL_SIDES);

    llSetLinkAlpha(RR3, 0, ALL_SIDES);
    llSetLinkAlpha(RR4, 1, ALL_SIDES);
}

step5 ()
{
    llSetLinkAlpha(LF4, 0, ALL_SIDES);
    llSetLinkAlpha(LF5, 1, ALL_SIDES);

    llSetLinkAlpha(RL4, 0, ALL_SIDES);
    llSetLinkAlpha(RL5, 1, ALL_SIDES);

    llSetLinkAlpha(RF4, 0, ALL_SIDES);
    llSetLinkAlpha(RF5, 1, ALL_SIDES);

    llSetLinkAlpha(RR4, 0, ALL_SIDES);
    llSetLinkAlpha(RR5, 1, ALL_SIDES);
}

default
{
    state_entry()
    {
       llSetTimerEvent (0.0);
       num = 1;
       running = FALSE;
       stand();
       llSetLinkAlpha(1, 0, ALL_SIDES);
    }

    link_message( integer sender_num, integer n, string str, key id )
    {
        if (str=="stp")
        {
            num = 1;
            stand();
            llSetTimerEvent (0.0);
        }
        if (str=="fwd")
        {
            num = 2;
            llSetTimerEvent (0.3);
        }
    }

    timer ()
    {
        if      (num == 2){ num = 3; step2 ();}
        else if (num == 3){ num = 4; step3 ();}
        else if (num == 4){ num = 5; step4 ();}
        else if (num == 5){ num = 2; step5 ();}
    }
}

Understanding the script …

Let’s take a look at this and see what’s up. It looks like there are five states for each leg. Looking at left front, for example, we see LF2, LF3, LF4, LF4, and LFSTAND. There’s a “stand” function, stand(), that sets alpha on for all the LFSTAND, RFSTAND, and so on, and sets alpha off on all the others.

But wait! As I look at these, I see we have names LF, RL, RF, and RR. I wonder if those stand for Left Front, Right Front, Right Rear, and ??? maybe Rear Left? My guess is that’s what’s going on, anyway. I’ll make a note of that odd name, and move on.

There are also a bunch of step() functions, each one setting one set of legs off, and one set on. I imagine that each function will turn off the previous leg setting and turn on the next, and that seems to be consistent, as, for example, step3() turns off the xx2 ones and turns on the xx3 ones. Makes sense. 

There’s some initialization, and a link_message to tell it to start and stop (the cart is controlled elsewhere), and a timer to move the legs. The timer just ticks through the four states 2, 3, 4,5 , over and over again. It all seems pretty sensible to me. I notice a running flag that is set but never checked. Also there is a Root variable that is never used. We’ll probably remove those real soon now.

We need tests …

When we make changes to improve this code, we want it to get the same answers! But how do we know that we have not changed the program? The official answer is: We test it. This program has no tests, which means that as we make changes to it, we can’t be certain that we haven’t broken it. There’s not much use to improving a design if we break the program in the process. So let’s think about how we could test this thing.

Most of the work is done in the step() functions, and in stand().  These functions just do a series of llSetLinkAlpha calls. We could build an object with a bunch of prims, and run these functions, and see if the alphas were set, but that seems like more trouble than it is worth.

Whenever we set out to refactor some existing code with few tests, we find ourselves thinking “This is too hard to test,” and we are tempted to just go ahead and do the work. After all, we’re all very skilled here, and we probably won’t make any mistakes. Yes, well. My experience is that while I may be skilled, I do make mistakes. In something like this, mistakes will be hard to spot and harder to figure out just what is going on. So I try hard, especially at the beginning, to figure out a way to test.

Let’s think about a test for step2(). No, step2() is more complex than some of the others. Let’s do step3(). It looks like this:

step3 ()
{
    llSetLinkAlpha(LF2, 0, ALL_SIDES);
    llSetLinkAlpha(LF3, 1, ALL_SIDES);

    llSetLinkAlpha(RL2, 0, ALL_SIDES);
    llSetLinkAlpha(RL3, 1, ALL_SIDES); 

    llSetLinkAlpha(RF2, 0, ALL_SIDES);
    llSetLinkAlpha(RF3, 1, ALL_SIDES);

    llSetLinkAlpha(RR2, 0, ALL_SIDES);
    llSetLinkAlpha(RR3, 1, ALL_SIDES);
}

OK. When step3() runs, it is going to turn on LF3, RL3, RF3, RR3, and turn off LF2, RL2, RF2, and RR2. Looking back at the numbers LF3 and all are defined to, it is going to turn on 7, 10, 11, and 20, and turn off 9, 14, 12, and 21. I’m going to just make up what a simple test for step3() might look like. This is often the way one does this: just say what you wish you could say. There’s a pattern for how this is done, called Arrange, Act, Assert. The test has three parts, first arranging things for the test, then doing the operation you’re testing, then asserting that the results are correct.  I often like to start with the assertion.

Write the test …

OK, here’s my cunning plan. I’m going to do whatever is necessary to make the turning on and turning off create two lists, one for the link numbers that were set to alpha 1, and one for the ones set to 0. Then I’ll check those. A quick check of the LSL wiki tells me that you can’t compare two lists for equality, but provides a very cryptic function to do it. I tested it briefly and it seems to work. Maybe we’ll try to figure it out later. Here it is:

integer listsAreEqual(list a, list b) {
    integer aL = a != [];
    if (aL != (b != [])) return 0;
    if (aL == 0)         return 1;

    return !llListFindList((a = []) + a, (b = []) + b);
}

OK, so here’s what I’d like my test to look like:

testStep3() {
    step3();
    assertEqual(on, [7, 10, 11, 20]);
    assertEqual(off, [9, 14, 12, 21]);
}

I haven’t written assertEqual yet, but I have in mind that it will use the listsAreEqual function and print a message if the two lists aren’t equal.

Make it compile …

To proceed, I’ll just paste these items into the script, and do what is necessary to make them compile. Then I’ll run the test, probably using the touch event, since it’s not in use now. The test should run but fail. Then I’ll make it work, which should mean that the changes I make are safe. Here goes:

list on;
list off;

testStep3() {
    step3();
    assertEqual(on, [7, 10, 11, 20]);
    assertEqual(off, [9, 14, 12, 21]);
}

integer listsAreEqual(list a, list b) {
    integer aL = a != [];
    if (aL != (b != [])) return 0;
    if (aL == 0)         return 1;

    return !llListFindList((a = []) + a, (b = []) + b);
}

assertEqual(list a, list b) {
    if ( listsAreEqual( a, b ) ) return;
    llSay(0, "Lists are not equal, " + (string) a + " != " + (string) b);
}

You can see that I added two global variables, on and off, and wrote the assertEqual function. Now I’ll add a call to testStep3() in a touch_start event, and it should fail:

    touch_start(integer ignored) {
        testStep3();
    }

Make it fail …

It’s actually important to see the test fail. That tells us that we’re actually testing something. If we write a test that shouldn’t work, and it does, that is very interesting. So I always try to make sure they fail. As planned, thisone  fails, looking like this:

[19:10]  Object: Lists are not equal,  != 7101120
[19:10]  Object: Lists are not equal,  != 9141221

The formatting there is pretty awful and on another day I would stop to fix that. But I’m on a roll now so I’ll put that off and move to make this first test run. Then, I think we’ll call it a night and continue on this tomorrow.

Make it succeed …

Look at step3(). It makes those repeated calls to llSetLinkAlpha, sometimes with a parameter of 1 and sometimes with zero. There is duplication there, and as you know, I am sworn to remove all duplication as part of improving code. I can kill two birds here by writing a couple of functions to turn alpha on and off, and in those functions do the calls to llSetLinkAlpha, and also add values to my list. 

Before I go heads-down to do that, though, I am seeing an issue. I’ve been saying things like “turn on alpha” and such. These are not such good names: which way is alpha “on”? So I’m going to change my lists to “visible” and “invisible”, and name my new little functions something like setVisible and setInvisible. I notice in my test that I need to initialize those lists or they will grow every time I press the test button. Here’s the new test:

list visible;
list invisible;

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

No real change. I have a little Arrange part now, clearing the lists, and there are the renamed lists. Time to make it succeed. Here goes. Next stop I’ll show you what I did, unless I get in trouble. If I do, I’ll show you what I did anyway. :)

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

    setInvisible(RL2);
    setVisible  (RL3); 

    setInvisible(RF2);
    setVisible  (RF3);

    setInvisible(RR2);
    setVisible  (RR3);
}

Above is the change I made to step3(). I just imagined these two new functions setInvisible and setVisible, and used them. Notice that I removed the 0 and 1 parameters and the ALL_SIDES from the llSetLinkAlpha version, since those are constant and can be inside the functions. Now I’ll code those functions:

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

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

As you can see, I renamed the lists from on and off to visible and invisible. I also learned that passing the test silently didn’t make me happy, so I changed the definition of assertEqual to look like this:

assertEqual(list a, list b) {
    if ( listsAreEqual( a, b ) ) {
        llSay(0, "success");
    } else {
        llSay(0, "Lists are not equal, " + (string) a + " != " + (string) b);
    }
}

That’s all I had to do, and now my tests run. The results look like this in my transcript:

[19:28]  Object: success
[19:28]  Object: success

Not bad for an hour’s work! I’ve got one test running, and I’ve actually implemented some useful functions that remove duplication from the code. I’m going to call this part done and turn in for the night. I’ll come back, probably tomorrow, and do the next bit. I’m hoping I can finish it all up, but we’ll take a look in the clear light of day. For now, thanks for reading!