Something To Do

It's the last night at Lake Okoboji. Brother Pat is making phone calls, Tom is reading down on the ground level, Dick is reading on the deck. My wife Ricia and sister-in-law Kathie are playing some strange dominoes game. I've finished the book I was reading, and the PC magazine that I brought along. No one to talk to, nothing to do. I think I'll refactor the code a bit, just for something to do. Let's take a look. Here's the method that handles Enter:

        public void Enter() {
            int cursorLine = CursorLine();
            String[] newlines = new String[lines.Length+2];
            for(int i = 0; i <= cursorLine; i++)  {
                newlines[i] = lines[i];
            }
            newlines[cursorLine+1] = "";
            newlines[cursorLine+2] = "<P></P>";
            for (int i = cursorLine+1; i < lines.Length; i++) {
                newlines[i+2] = lines[i];
            }
            lines = newlines;
            selectionStart = NewSelectionStart(cursorLine + 2);
        }    
	

Here are some of the ideas that went into this code:

• Handle the Enter key;
• Change the lines table to include a blank line and a P tag line after the line that contains the cursor;
• Figure out which line contains the cursor;
• Copy the lines from the beginning through the cursor line to the output;
• Put a blank line into the output;
• Put a P tag line into the output;
• Copy the lines from after the cursor through the end to the output;
• Place the cursor in the middle of the P tag;
• Remember the output lines;

You might be wondering why I'm mentioning the inclusion of the blank line and the P tag twice. The first mention is the purpose of the whole method. The second is a description of what some lines of code do.

Commenting for "Clarity"

Often we are told to comment our code to make it more clear. I think just for fun, I'll comment this code, to show where these ideas are hiding. We'll see whether this makes the code better, or not. Then we'll see whether we can do better than just putting comments in to make things more "clear".

        
// 
        // Handle the Enter Key
        //
        public void Enter() {
        //
        // On Enter, we change the TextModel lines to insert, after the line containing 
        // the cursor, a blank line, and a line with <P></P>. We set the new cursor
        // location to be between the P tags: <P>|</P>.
        //
        // calculate which line has the cursor
            int cursorLine = CursorLine();
        // allocate a new line array with two more lines
            String[] newlines = new String[lines.Length+2];
        // copy line 0 through cursor line to output
            for(int i = 0; i <= cursorLine; i++)  {
                newlines[i] = lines[i];
            }
        // insert blank line
            newlines[cursorLine+1] = "";
        // insert P tag line
            newlines[cursorLine+2] = "<P></P>";
        // copy lines after cursor lien to output
            for (int i = cursorLine+1; i < lines.Length; i++) {
                newlines[i+2] = lines[i];
            }
        // save new lines as result
            lines = newlines;
        // set cursor location
            selectionStart = NewSelectionStart(cursorLine + 2);
        }
	

Now if your eyes are sharp, you noticed that there is a typo in [at least] one of those comments. I noticed that while I was typing it in, but decided to leave it to remind us all that I might not have noticed it. Comments aren't checked by the compiler, and they don't have to reflect reality. However (you'll have to take my word for this), I did rebuild the program and run the tests after putting these comments in, and it did compile and run even with that mistake in the comment. <smile/>

Is This Really More Clear?

We really need to ask ourselves whether this is better code. Frankly, I'd rather deal with this method in its first form rather than the second. Your mileage may vary on that. But let's see where we wind up before we finally decide.

Commenting Means "Not Done"

Kent Beck taught us that if the code needs a comment, we should take that as a sign that the code isn't done yet: either it doesn't work, or it isn't good enough. Let's see whether we can get rid of some of those comments, making the code so good that it really doesn't need them.

Easy Pickin's

Let's start with something easy, that first comment, "Handle the Enter Key". We're trying to express the idea that the method named "Enter" handles the processing of the Enter key. I like the word "process" better than "handle". So I think maybe I should remove that first comment, and rename the method from "Enter" to "ProcessEnterKey". But wait. The next comment points out that what we do on Enter is to insert a blank line and a P tag. Maybe we should deal with that instead. The code that calls "Enter", in the KeyDownHandler, looks like this:

      if (kea.KeyCode == Keys.Enter) {
        model.Enter();
        kea.Handled = true;

I'm torn between two ways to handle this issue. One is to rename the Enter method to ProcessEnterKey, but that will leave the next concern to be expressing insertion of the P tag. And we need to decide who should know that Enter means to do that. I'm inclined to think that the TextModel doesn't know why it is doing this, and that we should rename this method for what it does, not why or when it does it. So I'm going to rename the method to "InsertParagraphTag", at least for now. So the KeyDownHandler code becomes:

      if (kea.KeyCode == Keys.Enter) {
        model.InsertParagraphTag();
        kea.Handled = true;

And the Enter method looks like this:

    public void InsertParagraphTag() {
    //
    // On Enter, we change the TextModel lines to insert, after the line containing 
    // the cursor, a blank line, and a line with <P></P>. We set the new cursor
    // location to be between the P tags: <P>|</P>.
    //
    // calculate which line has the cursor
      int cursorLine = CursorLine();
    ...
    }

My first inclination was to remove that whole first comment, because I expect to express all those facts by the time I'm done. But I'm looking ahead, based on a lot of experience. Let's just see how much of the comment we can legitimately remove. I've already removed the part about processing the enter key. That fact is expressed in the code from KeyDownHandler, which pretty clearly says that if the key code is enter, we should InsertParagraphTag. I feel that a comment saying that wouldn't help anyone. In the InsertParagraphTag method itself, can we remove some or all of the long comment? What if we named the method InsertBlankLineAndParagraphTagAndSetCursorBetweenParagraphTag? Frankly I don't think that would help.

I feel that the essence of this method is insertion of the paragraph tag, and that the details don't need to be in the name. We might decide not to include the blank line, for example. So let's wait and see.

Well, the easy pickin's turned out to be easy enough, but you see that there was some thinking that went into just that simple Rename Method refactoring. We had a couple of approaches we could have taken to the naming, and we're still not entirely satisfied that we've got it good enough. Let's move on.

The Cursor Line

Well, the next line and comment are about calculating which line has the cursor. I like the variable name OK, "cursorLine", but I'm not so happy with the method name, "CursorLine". How about we call that method "FindLineContainingCursor", or "LineContainingCursor", or something like that? I think I'll call it LineContainingCursor. So I'll change the line that sends the message and the line that defines the method. So now the line has no comment, and it reads:

    int cursorLine = LineContainingCursor();	
		

Things are a bit better now. I'll show the code as it now looks, and then I'm going to bed. Important point here: I know I'm not done refactoring this code, but if there was a need to release it to the code base, I certainly could, because it is better than it was, and it is running all the tests. I might prefer to hold it out, but if I was working in a team environment, I might do better to keep it in the repository in case someone else needed to work on it. I believe that the best approach is to keep the best possible code, and the most current code, in the repository at all times. Anyway, here's where we are:

    public void InsertParagraphTag() {
    //
    // On Enter, we change the TextModel lines to insert, after the line containing 
    // the cursor, a blank line, and a line with <P></P>. We set the new cursor
    // location to be between the P tags: <P>|</P>.
    //
      int cursorLine = LineContainingCursor();
    // allocate a new line array with two more lines
      String[] newlines = new String[lines.Length+2];
    // copy line 0 through cursor line to output
      for(int i = 0; i <= cursorLine; i++)  {
        newlines[i] = lines[i];
      }
    // insert blank line
      newlines[cursorLine+1] = "";
    // insert P tag line
      newlines[cursorLine+2] = "<P></P>";
    // copy lines after cursor lien to output
      for (int i = cursorLine+1; i < lines.Length; i++) {
        newlines[i+2] = lines[i];
      }
    // save new lines as result
      lines = newlines;
    // set cursor location
      selectionStart = NewSelectionStart(cursorLine + 2);
    }

Here's what I'll be thinking about for next time. The next few patches of code are dealing with creating a new line array and then filling it in. I will want to be using Extract Method to make them more meaningful, with names like "CopyThroughCursor", "InsertPTag", "CopyAfterCursor", or something like that. I note that to do that, I probably won't want the new lines to be a local variable, because the individual methods will each want to process the newlines array. I can make it a member variable easily enough, but I don't feel entirely comfortable with that: a guideline that I use is that all the variables in an object should change at the same frequency, and it feels to me that lines changes at the beginning and end, and newlines changes a a lot, in the middle.

Also, there is this class in C# called ArrayList, which is like an array except that you can insert into it and such. I'm tempted to change TextModel to use ArrayList. But I might have to know more about ArrayList before I can decide on that.

Digression: Doing It Right

At this point, some readers are probably saying something like "If you had spent more time thinking about this at the beginning, you wouldn't have to be doing all this changing now". And it's true, if we had thought a long time at the beginning, and if we were very smart, I might not be doing this changing now. We might also not have any running code at all. Chet and I are very comfortable working this way, through lots of practice. I like it that I can walk away tonight, and not address this problem again until I'm home two days from now, maybe with Chet's help, and that the code is working fine now, so that what I'm working on is cosmetic, not functional. But you might find comfort working with more thought up front.

I'd urge you, though, to push the refactoring approach all the way to the limits, like we're doing here, at least for a while. That way, you'll know what it really feels like rather than be guessing. You may be feeling that I'm doing a lot of rework. But that's not what's really happening: Chet and I didn't do much thinking up front about what to call these methods and lines of code -- we deferred that thinking almost entirely, and we're doing it now. We agree that time must be spent thinking about these matters. We're just saying that we can defer that thinking until later. Later (that is, now) we know more about what the code really wants to be, so we are better qualified to give things good names and arrange things for best effect.

If we were to stop with the code as originally written, that could be bad: it's not very clear and it might be hard to maintain. If we were to think more up front, we might make better decisions, but they would be made in the absence of the learning we gained by making this code work.

There's an old saying in programming: "Make it work, make it right, make it fast." That's what's happening here. We have made it work. Now we're in the process of making it right, and if we need to, later, we'll make it fast. There's nothing new under the sun: the XP programming style is "Make it work, make it right, make it fast." We just know a bit more about how to go about it.

You think on that. I'm going to bed now, and I'll work on this more when I get back to Michigan.