Refactoring V: Will This Never End?

 

Here I am in my sky bedroom, getting ready for a dance date. Now that I’m all cleaned up, I’ve got a little time to do some more work. Let’s see what happens.

After my break, I’d like to pick up where I left off. In this case, I can do it by looking at the article. But what if I were just scripting, not writing an article? What then?

One way to remember where we’ve left off in our program, when we’re developing it, is to write a test that doesn’t pass yet, and then save the script. That way, when we come back, we can run the tests and see what fails, and pick up there.

It would have been possible to do a similar thing here: I could have partially converted the next function, leaving the tests broken. That’s not such a cool thing to do, in my opinion. The reason is that I’ll have taken a working program and broken it, and then saved it. That’s not quite the same as taking a mostly working program and writing a test that shows that it isn’t done yet.

Anyway, this time we have my notes. I’ve now converted step3, step4, and step5. The two that remain, step2, and stand, are a bit more complicated. Here they are, unconverted:

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

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

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

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

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

These are both troubling. The earlier functions, step3 and step4, both proceed simply: set something invisible, set something else visible, once for each leg. These two are different from that, and from each other. The stand function sets the standing position visible, then makes all the other settings invisible. The step2 function, on the other hand, sets visible and then invisible, not the other way around, plus it has a few extras in there. In addition, it has those odd comments identifying the stand positions (I guess).

Now imagine that you just came across all this code. You would be wondering why these two functions do extra things, and why they do things in a different order. We can guess why stand does extra work: probably the horse can move from any given step to a standing position, based on when other things happen. So stand does need to turn off all the other possibilities. What about step2? My guess is that step2 occurs in two different cases, once when leaving the standing position, and once when leaving step5. Since it doesn’t know which is the case, it is set up to do both.

But why do these two functions set visible first, and then invisible, while the others set invisible first? Why do they solve a similar problem–not knowing where we came from–in different ways? Is it just random variation, or is there a reason? This is one of those times when you wish you had the phone number of the author so you can ask “What the heck is going on here???” The code has let us down a bit: it’s not entirely clear, nor are there any helpful explanatory comments. Pretending that I can’t just ask Dizzi, I’ll have to decide what to do and take my chances that there isn’t something going on here that I don’t understand.

I can see some different ways to go. I could preserve this new visible-first functionality in these, making a new function like changeVisibility that works the other way. I could reverse these two to invisible first, changing their tests and then refactoring them. Either way, I’m in trouble. If I reverse them, I am changing the design while refactoring, and that is risky. I do have my tests, so it isn’t terrible. If I don’t reverse them, then I’ll have two complex functions, maybe even three, instead of just the one that I’m hoping for.

It’s a judgment call, and I have no judgment that I’m aware of. I’m going to take a chance, change the order of the tests for those two functions, change the functions, and then go ahead. Here are the tests:

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

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

Notice in the final assertEqual, that these tests assume visible first, then invisible, alternating, while the other tests went invisible first. (We know this because the final assert goes 9,4, not 4,9.) So it should be possible to rewrite the tests easily, make them fail, then make the functions work, then refactor them. “Easily”. Yeah.

Both of these tests show more values turning invisible than they do visible. My plan will be to alternate invisible / visible until the visibles run out, then pick up the rest of the invisibles. So testStep2 gets rewritten like this:

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

I get one error, as expected:

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

Now if I change step2 judiciously, we should be OK again.

Arrgh. That turned out not to work. I rearranged the test items incorrectly, so that to make them work I had to break the pattern that the setInvisible / setVisible proceeded one leg at a time.

Now my plan was, as I said, to move the extras to the end. But since the extras were interleaved, I moved the wrong ones. My test is wrong. Back to the drawing board. I’m going to put the test and code back the way it was and proceed in smaller steps.

Backing up …

I’ve put the function step2 and the test testStep2 back the way they were, and the tests run again. Now to rethink the pattern of change for the ordering test.

Maybe you’re wondering why I’m trying to rethink it at all. Why don’t I just change the code, moving two of those extra lines down to the end, run the tests, see why they fail, and copy the answer into the test? Well, I could. But then, if I make a mistake in moving the lines, I’ll perpetuate that mistake in my test. That seems wrong to me. So I’d prefer to reason to the test than just copy what may be a mistake. I’ll try again, but it’s tricky thinking about it.

What is happening is that the invisibles are coming second rather than first, and that there are two extra ones that I’m going to move to the end. So … how about this for the test:

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

… and this for the code:

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

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

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

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

    setInvisible(LF5);
    setInvisible(RL5);
}

That works! I created the test by hand with the expectation that I’d move those two setInvisible lines to the bottom, and it worked. And now all the paired visible / invisible lines match up. Looks better.

Now I need to take a break. I have a dancing date. Next step, after looking around, will be to refactor this newly rearranged code, and make it work. First, let me sum up a bit.

Where are we …

We have one more function, step2, ready to convert to the new scheme. And I think we’ve learned a bit about how to do it. There will be a little issue of the extra elements to turn off, to be resolved somehow. I expect that to go easily, and that the final function, stand, should follow the same pattern. We’ll see!

This has dragged on a bit, but it’s because of all the article writing. The script changes are each taking me just minutes to do. But my purpose here is to give you a sense of how this sort of thing might be done, and what you might think about, not just to present the improved code as a sort of miracle, like this one:

 

I don’t like reading articles that show some kind of perfect code without telling me where it came from and how I might do a similar thing. So here, I’m trying to explain what I think as I do it.

I hope you’re finding it useful or at least amusing. If so, please let me know. If not, keep it to yourself.

For now, I’ve got a dance date to keep. See you later! Final installment coming, I hope!