Refactoring VIII: Real Close Now …

Behind the Electric Fence
Behind the Electric Fence

Here I am looking for some privacy behind my electric fence. I’m going to push this leg-list idea just a bit further, then we’ll see if we like it. Remember that the data looks like this:

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;

list leg_position_2 = [LF2, RL2, RF2, RR2];
list leg_position_3 = [LF3, RL3, RF3, RR3];
list leg_position_4 = [LF4, RL4, RF4, RR4];
list leg_position_5 = [LF5, RL5, RF5, RR5];

list legs_standing  = [LF_STAND, RL_STAND, RF_STAND, RR_STAND];
list not_standing   = [LF2, LF3, LF5, RL2, RL4, RL5, RF3, RF4, RF5, RR2, RR3, RR4];
list stands_plus_5  = [LF_STAND, RL_STAND, RF5, RR5, LF5, RL5];

The main thing I don’t like here is that there’s a lot to look at. All those initial values, then the lists. In addition, the data is somewhat redundant, in that values like LF2 and such mean LeftFront2, and much of that same meaning is included in the lists. It’s not a clear call. Anyway, I’m going to fold the constants into the list and see if I like the look of that better. I’m confident in my tests, so I should be able to do this pretty safely. We’ll see. :)

list leg_position_2 = [ 9, 14, 12, 21];
list leg_position_3 = [ 7, 10, 11, 20];
list leg_position_4 = [ 4, 15, 17, 19];
list leg_position_5 = [ 8, 13, 16, 18];

list legs_standing  = [ 4, 10, 12, 18];
list not_standing   = [ 9,  7,  8, 14, 15, 13, 11, 17, 16, 21, 20, 19];
list stands_plus_5  = [ 4, 10, 16, 18, 8, 13];

OK, that was easy enough, and the tests are all running. Now do we like it better, or do we not?

The lists are pretty arbitrary-looking with just numbers in them, but the code is much more compact. In the first version above, there are more lines but we can imagine that we are getting a little more of the pattern of what’s going on in the lists. Possibly with improved names, we’d get even more. Let’s just compare a few lines back to back:

integer LF2       = 9;
integer LF3       = 7;
integer LF4       = 4;
integer LF5       = 8;
integer LF_STAND  = 4;
...
list leg_position_2 = [LF2, RL2, RF2, RR2];
list leg_position_3 = [LF3, RL3, RF3, RR3];
list leg_position_4 = [LF4, RL4, RF4, RR4];
list leg_position_5 = [LF5, RL5, RF5, RR5];

Compared to just

list leg_position_2 = [ 9, 14, 12, 21];
list leg_position_3 = [ 7, 10, 11, 20];
list leg_position_4 = [ 4, 15, 17, 19];
list leg_position_5 = [ 8, 13, 16, 18];

Darn. I think there’s no question about it. The old way makes the leg lists make more sense. If we would rename those variables a little, at least rename RL to LR (LeftRear), it would make even more sense.

My judgment: in this case, the longer data makes things more clear. Put it back the way it was. Renaming that one set of variables to LR instead of RL, here’s what we have:

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

integer LR2       = 14;
integer LR3       = 10;
integer LR4       = 15;
integer LR5       = 13;
integer LR_STAND  = LR3;

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

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

list leg_position_2 = [LF2, LR2, RF2, RR2];
list leg_position_3 = [LF3, LR3, RF3, RR3];
list leg_position_4 = [LF4, LR4, RF4, RR4];
list leg_position_5 = [LF5, LR5, RF5, RR5];

list legs_standing  = [LF_STAND, LR_STAND, RF_STAND, RR_STAND];
list not_standing   = [LF2, LF3, LF5, LR2, LR4, LR5, RF3, RF4, RF5, RR2, RR3, RR4];
list stands_plus_5  = [LF_STAND, LR_STAND, RF5, RR5, LF5, LR5];

You’ll notice that I made the xx_STAND definitions all symbolic instead of literal. The reason why? Doing it this way calls out that those values are duplicated, which is an important fact.

What’s left to think about? Well, the last two lists, not_standing and stands_plus_5 are not obvious. If you’ll think about it a moment, not_standing is just all_legs – legs_standing, if we could subtract lists. And stands_plus_5 is legs_standing + [RF5, RR5, LF5, LR5]. That one is particularly cryptic, because of the fact that its used to transition to step2, which can be entered from 5, or from stand. I’d almost rather not think about that.

Still, if lists could be added and substracted, I would consider calculating those values in the program rather than calculating them by hand as Dizzi originally did. But the code for doing that is a bit ugly in LSL, so I don’t think it would add to clarity.

Well. We might be done. I’m taking a look at the whole program now, and I don’t see any improvements–or attempted improvements–that I want to make.

What’s left? Well, we have one thing to worry about, which is the extra code we inserted for testing, especially the code that adds values to the testing lists:

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

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

Basically we can delete those lines, or comment them out, or we can set a flag when testing and clear it otherwise, so that the lists are only added to when we’re testing, not when our horse is just running. Horse, running. Get it? Never mind.

There’s also the testing code itself to consider. LSL scripts take up space and there isn’t much available. What should we do? Judgment call.

There are a couple of things that I’m pretty sure of, though. First, if we remove all the testing code, we’ll never put it back. Next time we work on the program, whenever that is, we’ll have lost it. Second, I happen to know that Dizzi intends to make some changes to this code, to make the horse jump. In fact, she has made those changes to her version already. Will she pick up this version and use it instead? Should she? We’ll talk about that in the summary below. Here’s what I’m going to do: I’m going to comment out all the test code, but leave it in for history. In a real program it would be in a separate module but we don’t have that luxury here.

On another day, I might remove it. On yet another day, I’d leave it in just disabled. Today, I’m for commenting it out. I’ll put the final program at the very end of this article, after the summary. And here’s the summary:

Summary

The point of this long series of articles was to experiment with refactoring in LSL, to learn  how to do it, and to show you how I would do it. Your job, as always, is to take what you see here and pull whatever value you can from it.

My own assessment of this experiment is that it went pretty well. There were no long periods of confusion, and I’m very sure that the program is doing exactly what the original did except in the few places where I really intended to change it. I feel that the code is more readable now, and more consistent. (It’s OK if you don’t agree with that: whenever we improve any code, we should do it to make it more like our own, or our team’s own, standard of goodness. Now, of course, if you don’t like my standard of goodness, I think you’ve got some learning to do but I’m sure you feel the same way.)

Here’s my list of the important things in the process I used here:

  1. Surround the code with tests. These are called “characterization tests”. They characterize the code, making sure that it continues to do what it originally did. With these tests in place, we can change anything, confident that if we break it, the tests will tell us.
  2. Change things in tiny steps. When we write or change code, we probably make mistakes at a roughly constant rate. We might even make more mistakes as we go along, from tiredness, or from trying to remember too many things at once. So we always make little changes, and run the tests again. This way, when we do make a mistake, it’s easier to find and remove.
  3. Look for and remove duplication. Looking back over these articles, notice how many times the improvement I found was about removing duplication. Multiple occurrences of the same or similar lines of code are opportunities to consolidate. Consolidating code makes most of it simpler, and concentrates any necessary complication in localized places.
  4. Improve the code’s expressiveness. Often, just renaming a variable or function can make the code easier to understand. I picked the names like leg_position_2 to help me, and future readers, to figure out what’s going on.

So that’s the approach. I found this whole exercise almost relaxing. I felt no stress that I might break the program, because I had my tests. I went in little steps, so I could stop whenever I was hungry or someone wanted to take me dancing. And even though it took ages to write these articles (and, I’m sorry, to read them), the actual process of coding was probably less than one-tenth of the time.

So, there you are. We’ve refactored this code, made it better by some standards, and certainly quite different, and we’re sure it is still functioning as we want it to. Code improvement, safely and with no debugging. Life is good.

I’ll put the new code, and the old code, right down below. Thanks for reading, and bye for now!

Final Code (see below for original code)

integer Root      = 1;
integer running;
integer num;

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

integer LR2       = 14;
integer LR3       = 10;
integer LR4       = 15;
integer LR5       = 13;
integer LR_STAND  = LR3;

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

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

list leg_position_2 = [LF2, LR2, RF2, RR2];
list leg_position_3 = [LF3, LR3, RF3, RR3];
list leg_position_4 = [LF4, LR4, RF4, RR4];
list leg_position_5 = [LF5, LR5, RF5, RR5];

list legs_standing  = [LF_STAND, LR_STAND, RF_STAND, RR_STAND];
list not_standing   = [LF2, LF3, LF5, LR2, LR4, LR5, RF3, RF4, RF5, RR2, RR3, RR4];
list stands_plus_5  = [LF_STAND, LR_STAND, RF5, RR5, LF5, LR5];

stand () {
    changeVisibility(not_standing, legs_standing);
}

step2 () {
    changeVisibility(stands_plus_5, leg_position_2);
}

step3 () {
    changeVisibility(leg_position_2, leg_position_3);
}

step4 () {
    changeVisibility(leg_position_3, leg_position_4);
}

step5 () {
    changeVisibility(leg_position_4, leg_position_5);
}

// JR Implementation Starts Here

changeVisibility(list off, list on) {
    integer itemMax = 4;
    integer itemNumber;
    for ( itemNumber = 0; itemNumber < itemMax; itemNumber++ ) {
        setInvisible(llList2Integer(off, itemNumber));
        setVisible(llList2Integer(on, itemNumber));
    }

    integer offMax = llGetListLength(off);
    for ( itemNumber = 4; itemNumber < offMax; itemNumber++ ) {
        setInvisible(llList2Integer(off, itemNumber));
    }
}

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

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

// JR Implementation Ends Here

// JR Testing Starts Here

//list visible;
//list invisible;
//list ordering;

//assertEqual(list a, list b) {
//    if ( listsAreEqual( a, b ) ) {
//        say("success");
//    } else {
//        say("Lists are not equal, [" + llList2CSV(a) + "] != [" + llList2CSV(b) + "]");
//    }
//}

//say(string s) {
//    llOwnerSay(s);
//}

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

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

//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, [9, 4, 7, 10, 8, 12, 14, 18, 15, 13, 11, 17, 16, 21, 20, 19]);
//}

//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);
//}

// JR Testing Ends Here

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 ();}
    }

//    touch_start(integer ignored) {
//        testStep2();
//        testStep3();
//        testStep4();
//        testStep5();
//        testStand();
//    }
}

Original Code

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 ();}
    }
}

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s

Blog at WordPress.com.

Up ↑

%d bloggers like this: