Refactoring VI: How Many Roman Numerals Do I Know?

Here I am sitting in one of our cozy spots in Lexicolo, reading a book before starting this article. I’m hoping to finish up this time … will I?

When last we visited our intrepid heroine, she had just rewritten the test for step2 and made it work by reordering the actions to be consistent with other steps. Now she has to refactor the code to make it better, in the style of her other changes. We’ll watch. That’s all very nice for you, following along. But what about me? I’m the intrepid heroine and I have to actually make this happen. At least I get to do it in a nice location.

I was going to show you the test and code as they now stand, and I will. But I see another problem, so first let’s compare two tests:

testStep2() {
    setupTest();
    step2();
    assertEqual(visible, [9, 14, 12, 21]);
    assertEqual(invisible, [4, 10, 16, 18, 8, 13]);
    assertEqual(ordering, [9, 4, 14, 10, 12, 16, 21, 18, 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]);
}

Compare testStep3 with testStep2. In testStep3, notice that in the ordering test, the list starts with 9, the first item made invisible. But in testStep2, it starts with 9 (als0) but that’s the first item made visible. I forgot to test for doing invisible first, and of course, then I didn’t change the order. The test runs, but it’s still wrong. So first I change the test to reflect what we really want:

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

Notice that I just reversed the pairs, going invisible first. The test now fails, of course. Now all I should have to do is reverse the order of the items in the step2 function and all should be well again:

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

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

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

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

    setInvisible(LF5);
    setInvisible(RL5);
}

I did that, ran the tests and they all passed. Back where we intended to be.

Is there a lesson here?

When something goes wrong, I like to reflect for a moment to see if there is something to learn. One thing is that if I had been working with someone else, they might well have seen the problem sooner. Another is that I’m really glad I have my tests, because I can read them to see what is supposed to happen, notice if the test itself is wrong, then fix it and then fix the code, with a lot of certainty that I’m getting what I need.

Another lesson, though, may be that these tests, while solid, are a bit too hard to read. It may be that I should have seen sooner that there was something wrong with that one. I’ll make a note to think about that later. For now, we know the tests are solid, and I’ll proceed with the refactoring.

The cunning plan …

… is just to create the two in and out lists as before, then call changeVisibility. We expect that this will nearly work, but not quite, because as it is written, changeVisibility only processes four elements from each list, and we are going to have six elements in our off list. Like this:

step2 ()
{
    list off = [LF4, RL3, RF5, RR5, LF5, RL5];
    list on  = [LF2, RL2, RF2, RR2];

    changeVisibility(off, on);
}

I haven’t run the tests yet. What I predict is exactly one error, on the ordering list for testStep2. I expect the actual to be two items shorter than the expected. Let’s see. Here are the errors:

Lists are not equal, [4, 10, 16, 18] != [4, 10, 16, 18, 8, 13]
Lists are not equal, [4, 9, 10, 14, 16, 12, 18, 21] != [4, 9, 10, 14, 16, 12, 18, 21, 8, 13]

Yes, errors. I was right about the ordering list, but I forgot that the off list would also be too long. This is why we have tests, because our brains are often not up to the challenge of predicting exactly what will happen. Well, my brain, anyway. Yours maybe is.

In any case, we need to make this test run, and that will be done by making the changeVisibility function process the full extent of the off list. Let’s think how to do that. Here’s that function as it stands now:

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

Right now, the code assumes it just has four items and loops over them. It’s skipping the extra two entirely.  There are a couple of things we might do. Maybe we could change itemMax to be the larger of the two list lengths and keep processing. But that would make extra calls to setVisible, and the llList2Integer would call into the on list looking for things that aren’t there. What would it do? According to the documentation, it would return zero. We could check for that but it seems ugly.

Another possibility is to add another loop to the end of changeVisibility, from five to the end, just calling setInvisible. That’s a bit odd, I think, but it seems more clean to me. So I’ll add that:

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

That works just fine. All the tests run. Now we are left just with the stand function and its test. Starting out, they look like this:

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

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

OK, this is the big one. I’ve saved it till last because it is the hardest. Would you have perhaps started with it because it is the hardest, thinking that once you did the hard one, the others would be easy? I usually go the other way.  I start with the easy ones, learning as I go, perhaps changing my strategy as I go. By the time I get to the hardest one, I expect that my tests, my existing code, and my understanding will all be high, so that the hard one will be easy.

Generally speaking, it works for me. Now let’s look at what has to be done.

First of all this one starts with four setVisibles, then does all its setInvisibles. This is counter to our normal practice of alternately setting one invisible and one visible, repeating until done.  Notice that the test reflects that. The ordering list shows all the visibles, then all the invisibles. To be consistent with our others, we need to weave in the first four invisibles, with an invisible going first. Here’s what the new test should look like:

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

And here’s the reordered code supporting the new test:

stand ()
{
    setInvisible(LF2);
    setVisible(LF_STAND);

    setInvisible(LF3);
    setVisible(RL_STAND);

    setInvisible(LF5);
    setVisible(RF_STAND);

    setInvisible(RL2);
    setVisible(RR_STAND);    

    setInvisible(RL4);
    setInvisible(RL5);

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

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

The test passes. I’m getting good at this, which was what I was hoping before I had to do this most difficult one. Now let’s refactor to use the changeVisibility function. Again, it should be straightforward:

stand ()
{
    list off = [LF2, LF3, LF5, RL2, RL4, RL5, RF3, RF4, RF5, RR2, RR3, RR4];
    list on  = [LF_STAND, RL_STAND, RF_STAND, RR_STAND];

    changeVisibility(off, on);
}

And it was. It did take me three times to type it in correctly, but the tests all pass. Let’s review the functional code now, to see how the refactoring has proceeded so far:

stand ()
{
    list off = [LF2, LF3, LF5, RL2, RL4, RL5, RF3, RF4, RF5, RR2, RR3, RR4];
    list on  = [LF_STAND, RL_STAND, RF_STAND, RR_STAND];

    changeVisibility(off, on);
}

step2 ()
{
    list off = [LF4, RL3, RF5, RR5, LF5, RL5];
    list on  = [LF2, RL2, RF2, RR2];

    changeVisibility(off, on);
}

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

    changeVisibility(off, on);
}

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

    changeVisibility(off, on);
}

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

    changeVisibility(off, on);
}

Compare that to the same code from the original:

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

To my taste, the new way is better, because each function is shorter and more clear. However, this brevity and clarity does come at a price, the addition of three new functions, one of which, changeVisibility is rather complex. The new way does put all the complexity in one place but between the two loops and the list manipulation, there’s no doubt that this one spot is more complex:

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

I also added a little complexity for the testing, with the setting of the variables visible, invisible, and ordering. That’s all nicely hidden, but there are some issues with what I’ve done there. Can you see what they are?

If I leave that code in the setVisible and setInvisible functions, then when the horse runs, it will continually add items to those lists. After not many steps at all, those lists will fill up memory and the script will explode. We will need to do something about that if we decide to keep this code.

What??? IF we decide to keep it???

Well, yes, if. Refactoring is a process for improving the design of existing code. Sometimes we think we see a direction to go to improve the code, and along the way or at the end we decide that we haven’t improved it after all. That’s fine: it’s part of the learning process. So I am still prepared to toss all this code if, in the end, it doesn’t seem truly better.

Besides, the real point was to write these articles to help you understand what refactoring is and how we do it. This script is just an example.

Mind you, I’d rather have a good outcome that feels like an improvement. But I do try to remain open to better ideas, even if they are ideas from the past rather than the present. I think that’s the view we should have, assessing the code on its own merits, not loving it like our own precious little creation. If it’s ugly, I’m throwing it back.

One more observation …

Take a look at the lists used in most of the step functions. For example, this one:

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

    changeVisibility(off, on);
}

Hey! Step four is “turn off legs number 3, and turn on legs number 4”!!! Let’s rename the variables just to emphasize that:

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

    changeVisibility(legs3, legs4);
}

Now that’s starting to make sense! The easy functions will all turn out that way. The more complex ones, step2, and stand, look a bit different. That might make us fear to go forward but we’ve already been able to adjust the corresponding tests and improve that code.

Might it turn out that each of these functions can be expressed in terms of operations on lists that are representations of legs? If so, can we reduce those long lists of oddly named variables to convenient lists? And if so, might that make the script even simpler?

I think maybe it might. We’ll see in the next article. For now, it’s time to fix some dinner. Bye for now!