Wait! Check the comments on Refactoring V!
(Dizzi phones in long distance)
I think the ideal animation on a leg (assuming it takes a finite time to do it and may happen between two frames of display at 50 Hz) is to make the new leg position visible first, then immediately delete the old position in that same leg. This way at worse the horse appears to have more legs than normal which appears like a motion blur, but never appears to be missing a leg.
Just be thankful we are not implementing Sleipnier (Odins 8 legged horse)
Although if the lag is bad it may look that way;)
Oh wow. She wants a change! Doesn’t that always bug you, there you are with a perfect program and they want to change it? Well, refactoring is supposed to help with changes. Let’s see if it does!
Refactoring to good code can’t make us completely indifferent to changes, but it sure can help. Let’s look at what we would have to change in the original code:
paragraph
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 ()
{
setInvisible(LF2);
setVisible (LF3);
setInvisible(RL2);
setVisible (RL3);
setInvisible(RF2);
setVisible (RF3);
setInvisible(RR2);
setVisible (RR3);
}
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);
}Yikes. We have to go through all those lines, making sure that all the llSetLinkAlpha with a parameter of one (1) come before the ones with a parameter of zero (0). Not terribly scary but tedious and error prone. As we’ve mentioned before, some of the functions do what Dizzi wants, and some don’t. We’d have to be very careful.
What about in our new code? Here’s what we have to worry about:
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));
}
}We see right away that now we are doing exactly what she doesn’t want. We’re going invisible first, then visible. OK, no big deal: we’ll just reverse those two lines. Cool.
This is not a trick.
I did not invent this request for a change, I didn’t make it happen, I did not contrive it. But I’ll darn well take advantage of it!
Here’s a perfectly reasonable request to change our code, and two forms the code might look. In one form the changes are spread all over and are tedious and probably error-prone. In the other form, there is just one change to make. I think one is better than many.
Why does this happen? Because in refactoring, we try to remove duplication. When we remove duplication, we do so by centralizing the features of the program. This means that, more often, when a change comes in, it needs to go in only one or a couple of places.
A perfect example!
Not a bed of roses, however …
Actually, I’ve never been that sure about beds of roses. Roses have thorns. That could be less than comfortable. Anyway, everything isn’t entirely perfect even now. What about those tests? Remember that our tests have three parts. They check what was turned visible, what was turned invisible, and the order in which operations were done.
To play completely fairly, we need to update our tests to reflect this change in the desired order of things. This looks like a problem. Here’s a typical test, of which there are five:
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]);
}All of our tests have this assertion about the ordering of things in them. We require, at the moment, that the invisible and visible items alternate, invisible first, until only any extra invisibles are left, which are done last.
I remember that I had to redo some of those ordering tests already, back a few articles ago, and it was a pain. Now we have to do them all yet again. See? That’s why all this testing is a pain. Let’s not do it.
Hold on there, Skippy. We like these tests. They got us to our new design with no debugging at all, no worrying about whether we had what we needed. I don’t want to get rid of my confidence. I don’t want to have to do a bunch of debugging. I want my tests.
And I don’t want to have to keep changing them. There must be a lesson here.
Is there a lesson here?
Of course there is. Why do you think I do this stuff?
When we find that our tests need to change a lot, in irritating ways, is it a sign that we should do less testing? Not in my opinion. It is a sign that we should do better testing. Right now, we’re about to change the order in which our changeVisibility works, and we’re bugged by all those tests of the exact ordering of the calls to setVisible and setInvisible. We’ve already had to change them a few times, and it was a pain. But wait? Why are we testing the ordering over and over?
Here’s what I wrote when I originally built the ordering feature into the testing:
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?
Now what I did was to check, in each specific step test, whether the items were in the right order. It made sense to me at the time. But all the steps are done using the same function, changeVisibility. So, it should be enough to write tests for ordering by directly calling that function, and not check again in the detailed leg functions. This amounts to removing duplication. So first, I’ll write a new test that sends in two sets of numbers, and checks ordering:
testOrdering() {
setupTest();
list invis = [ 1, 2, 3, 4, 5, 6];
list vis = [10, 20, 30, 40];
list weave = [1, 10, 2, 20, 3, 30, 4, 40, 5, 6];
changeVisibility(invis, vis );
assertEqual( visible, vis);
assertEqual( invisible, invis);
assertEqual( ordering, weave);
}OK, that runs. Notice that we’re doing the invisibles first, and that I put a couple of extras in the list to be sure that feature keeps working: all the extra invisibles are supposed to be done last. Now, is this enough of a test? I think so. We could test things like not giving four elements and such, but I don’t see any point to that in this context. These are “characterization tests”, telling us what the system does, and locking it down. They are not comprehensive tests intended to find all kinds of obscure things. So I’m good with just this much testing, and now I feel comfortable removing the other tests on order. I wind up with this:
testStep2() {
setupTest();
step2();
assertEqual(visible, [9, 14, 12, 21]);
assertEqual(invisible, [4, 10, 16, 18, 8, 13]);
}
testStep3() {
setupTest();
step3();
assertEqual(visible, [7, 10, 11, 20]);
assertEqual(invisible, [9, 14, 12, 21]);
}
testStep4() {
setupTest();
step4();
assertEqual(visible, [4, 15, 17, 19]);
assertEqual(invisible, [7, 10, 11, 20]);
}
testStep5() {
setupTest();
step5();
assertEqual(visible, [8, 13, 16, 18]);
assertEqual(invisible, [4, 15, 17, 19]);
}
testStand() {
setupTest();
stand();
assertEqual(visible, [4,10,12,18]);
assertEqual(invisible, [9, 7, 8, 14, 15, 13, 11, 17, 16, 21, 20, 19]);
}
testOrdering() {
setupTest();
list invis = [ 1, 2, 3, 4, 5, 6];
list vis = [10, 20, 30, 40];
list weave = [1, 10, 2, 20, 3, 30, 4, 40, 5, 6];
changeVisibility(invis, vis );
assertEqual( visible, vis);
assertEqual( invisible, invis);
assertEqual( ordering, weave);
}Now the ordering is isolated into one test. I should be able to reverse the order of the statements in the changeVisibility and have only that one test fail. Let’s see … yes:
Refactoring for Dizzi IX: Lists are not equal, [10, 1, 20, 2, 30, 3, 40, 4, 5, 6] != [1, 10, 2, 20, 3, 30, 4, 40, 5, 6]
We see that the interweaving is reversed, just what we’d think. So now we change that one test, not by copying the result but by computing it by hand, and voila!
testOrdering() {
setupTest();
list invis = [ 1, 2, 3, 4, 5, 6];
list vis = [10, 20, 30, 40];
list weave = [10, 1, 20, 2, 30, 3, 40, 4, 5, 6];
changeVisibility(invis, vis );
assertEqual( visible, vis);
assertEqual( invisible, invis);
assertEqual( ordering, weave);
}So there we are, the order of setting visible and invisible is reversed, and we only had to move one line of code and fix one test.
Are We There Yet?
Well, yes and no. We have accomplished our goal, reversing the order of visibility setting, and we have done it without having to refactor all our tests. Instead we wrote one new simple one, simplified all the others, and then changed just that one test.
Probably it would have been better if I had changed the test first, seen it fail, then changed the code. I think that would be more like what the experts would have told me to do. It wouldn’t be right to go back and change the article though. I did what I did. This time I got away with it. If I hadn’t, we’d have learned another little lesson together.
But there is still some work to do. At the beginning of this article, though I didn’t make a big deal of it, I had to uncomment all those test-related lines. That was a pain, frankly. Now that I have the code back in production form, I need to get rid of all that testing-related stuff again. Another pain. And if there’s another change, more pain. I hate pain. So what’s best?
I spoke before of putting the list appends inside if statements instead of commenting them out. I think that would be better, so let’s do that. We’ll add a flag named, oh, testing, initialize it to false, and bracket the list accesses with tests of it.
That should make all the tests fail. Then we’ll set the flag at the start of the tests and clear it at the end. That will make them succeed. But wait! How will we test to be sure we clear the flag? Here’s one idea: we’ll write a test that tests the flag for being clear, and put it before, and after, the flag clearing. How about that? Should be solid.
testTestingFlagIsOff() {
if ( testing )
say("Testing flag is on and should be off");
else
say("Success: Testing flag is off");
}
touch_start(integer ignored) {
if ( llDetectedName(0) != "Janet Rossini" ) return;
testStep2();
testStep3();
testStep4();
testStep5();
testStand();
testOrdering();
testTestingFlagIsOff();
}OK. I just added the flag and the test, at the end of my test list, and it passes. It wouldn’t compile till I added the flag and would not have run correctly had I not initialized it appropriately. So far so good.
No, wait! My other tests are supposed to fail! I haven’t put the if statements around the list updates yet. Here’s that:
Now to add the flag setting, and put the flag-testing test on both sides of it:
setInvisible(integer link) {
llSetLinkAlpha(link, 0, ALL_SIDES);
if ( testing) {
invisible += [link];
ordering += [link];
}
}
setVisible(integer link) {
llSetLinkAlpha(link, 1, ALL_SIDES);
if ( testing ) {
visible += [link];
ordering += [link];
}
}Excellent! Now all my tests are failing, except for the flag-testing one. Now to update the test block itself. Updated, it looks like this:
touch_start(integer ignored) {
if ( llDetectedName(0) != "Janet Rossini" ) return;
testTestingFlagIsOff();
testing = TRUE;
testStep2();
testStep3();
testStep4();
testStep5();
testStand();
testOrdering();
testing = FALSE;
testTestingFlagIsOff();
}OK. That should be pretty bulletproof. If the testing flag is left on, the first test of it should fail. If it isn’t turned off by the tests, the second call of it should fail — and of course the next time we run the tests, it will fail.
I think I’m done now, except for commenting out the test calls in the touch_start. I’m kind of torn about doing that but otherwise they’ll be in there all the time. Not sure just where to go on that. Maybe I’ll just change the control line at the top and leave them in. Dizzi can remove them if she decides to use this code. Here’s that final change:
touch_start(integer ignored) {
if ( llDetectedKey(0) != llGetOwner() ) return;
testTestingFlagIsOff();
testing = TRUE;
testStep2();
testStep3();
testStep4();
testStep5();
testStand();
testOrdering();
testing = FALSE;
testTestingFlagIsOff();
}Now I think I’m done again. Let me sum up what we’ve learned and you can return to your normal life.
What Have We Learned?
Well, to my surprise, we received a change request that wound up pretty much justifying our code changes. In the old version, we would have had to change many lines in a manual and error-prone way. In our new version, we had to reverse the order of just two lines.
However, in our new version from article VIII, we would have had to change many lines in our tests, in a rather irritating way where we had to reorder test lists. So instead, we wrote just one test, and were able to remove all the other lines that were too dependent on our implementation. That meant we just had to change one test instead of many.
Then, I decided not to comment out all the tests, since in the course of a day I had to comment them out, comment them in, and then would have had to comment them out again.
Instead, I took a little more time, put if statements around the updating of the test lists, and wrote a new test to make sure that the testing flag was properly updated.
This was all pretty simple. Again it all went without any surprising errors and with no debugging.
I’m pretty happy with this. Dizzi’s desire to change how things work gave us a chance to see the benefits of refactoring in action.
I hope you’ve found this long series worth reading and that you’ll consider adopting the parts of these ideas that seem useful to you. In my RL programming, I try to work this way, and I find it very helpful. I think you will as well.
Thanks, and bye for now!
Valkyrie Janet 
Leave a Reply