Refactoring IV: Let’s Improve the Code

Here I am, sitting at my favorite table at Rossini’s, on the roof of the train station in Lexicolo. While I wait for the incredibly slow service, I’m going to start improving the code in Dizzi’s leg animation script. (I hasten to add that her original code is just fine, really. No offense intended to it or her.) Let’s see what happens.

In the previous article, we noticed the duplication of calls to setInvisible and setVisible, but because I wanted my tests to be more solid, we didn’t get around to removing it. Let’s look at a couple of examples:

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

We saw some related duplication in the data as well, with repeated blocks of five items:

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;

In that previous article, I considered a number of choices. To review, I was thinking there’d be a function like changeVisibility(list on,list off) that would take lists of values to change. Because I noticed that some of Dizzi’s functions turned off more than they turned on, I was thinking there should really be a third parameter, list extraOff, to handle those.

If you’ve been paying attention to my approach, you know I’m not going to start with that complex solution, even if that’s where I wind up. Instead, I’m going to convert just one of my step methods to use a list, to find out what it feels like to use a list here. My tests will protect me from making a mistake. (They may not protect me completely: I’m very good at making mistakes. We’ll see.)

Let’s start with step3().  It looks simple enough.

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

    setInvisible(RL2);
    setVisible  (RL3); 

    setInvisible(RF2);
    setVisible  (RF3);

    setInvisible(RR2);
    setVisible  (RR3);
}

As we look at the code above, we see that the “list” of values set invisible is LF2, RL2, RF2, RR2, and the ones set visible are LF3, RL3, RF3, and RR3. (I’m reminded that Dizzi commented that some of those names need changing. I’m not going to do that right now, in the middle of this. It would just mess up my momentum … I think. I’m just rote transcribing values now, so the meanings aren’t too important. Still, this is a bit risky, because with names that are less meaningful, I am less likely to notice a mistake. I’m going to count on my tests.)

So let me imagine this new function, changeVisibility(), and use it. First, I add the two lists, copying carefully from the code, by hand:

step3 ()
{
    list off = [LF2, RL2, RF2, RR2];
    list on  = [LF3, RL3, RF3, RR3];

    setInvisible(LF2);
    setVisible  (LF3);

    setInvisible(RL2);
    setVisible  (RL3); 

    setInvisible(RF2);
    setVisible  (RF3);

    setInvisible(RR2);
    setVisible  (RR3);
}

This compiles, and the tests run. Actually it compiles the second time. The first time I missed the square brackets. Now I just replace all the set calls with a single call to the new function, which I haven’t written yet. Remember that this is called “programming by intention”. I intend to write this function. My function here intends to “change visibility”:

step3 ()
{
    list off = [LF2, RL2, RF2, RR2];
    list on  = [LF3, RL3, RF3, RR3];

    changeVisibility(off, on);
}

And I build the new function. I decided to put it off in a space just for my new code:

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

// JR Implementation Ends Here

The tests all run! Ha! Perfect!

No, wrong. I thought of this as I went forward. The approach I was taught for this kind of work, Test-Driven Development, says that first I should see a test fail, then see it succeed. If it succeeds right away, it is possible that the test itself isn’t strong enough, or isn’t even used. So I just commented out the set lines in the code above and re-ran the tests. Three of them failed.

This has taught me something else, which is that I don’t like my tests just saying success or lists are not equal, because I can’t tell which ones failed without counting the lines. I’ll consider fixing that soon. For now I’ll check and see which one is failing by looking.  And just as we’d hope, it is testStep3. I uncomment the set lines and it works again.

So now I have converted one step to using lists. The changeVisibility function is a bit complex, but not bad, and the step3 function is less complex than it was. I can foresee that as I go forward, I’ll need to add that third list, but that should be fairly simple.

It will not be entirely simple, however. In some of those future steps, the lists will not all be the same length. So when we add the third parameter, and maybe before that, the changeVisibility function will absorb some more complexity. We’ll have to keep an eye on that.

Also, looking forward, once we get these lists inside each function, I foresee that we will have an interesting situation. You can think about that and we’ll deal with it when we get there.

When we get there?

Does that trouble you? Are you thinking that you were taught to figure everything out before you start, so that you won’t make any mistakes and have to do things over? Well, I was taught differently. I’ve worked both ways and I like this one better. As we continue on, notice that I’m not really doing much over, and that each step is quite simple. Meanwhile, I’m not thinking too hard: after all, I am writing this article at the same time.

Watch what we do, and make your own decision about what to try.

For now, I’m going to convert one more function to the new way. My target is step4. I might decide to do step5 at the same time. Uh oh. I feel myself rushing. I have to leave soon, and I’m trying to hurry up to get more done. Bad Janet. Stay steady, don’t rush. One more.

step4 ()
{
    list off = [LF3, RL3, RF3, RR3];
    list on  = [LF4, RL4, RF4, RR4];

    changeVisibility(off, on);
}

That one works. Easy-peasy. One more, step5:

step5 ()
{
    list off = [LF4, RL4, RF4, RR4];
    list on  = [LF5, RL5, RF5, RR5];

    changeVisibility(off, on);
}

That one works too. Time to take a break. I’ll pick up where I left off, later tonight. Remind me to talk about picking up where one leaves off. Bye for now!