Chapter 12: Adding the Section Tag, Part II
We ll continue refactoring our TextModel. Stand back, I don t know how good this thing gets!
Reviewing the Code
The TextModel.cs file was posted at the end of Chapter 11, Adding the Section Tag, Part I. We can still see lots of duplication in that code. There seems to be a general confusion between whether we want the lines to be an Array or an ArrayList, for example. That should be cleaned up. And here are two methods that are clearly duplication and that I think we should go after:
public void InsertParagraphTag() {
if ( lines.Count == 0 ) {
lines.Add( "<P></P>" );
selectionStart = 3;
return;
}
lines.InsertRange(LineContainingCursor()+1, NewParagraph());
selectionStart = NewSelectionStart(LineContainingCursor() + 2, "<P>");
}
public void InsertSectionTags() {
if ( lines.Count == 0 ) {
lines.Add( "<sect1><title></title>" );
lines.Add( "</sect1>");
selectionStart = 14;
return;
} lines.InsertRange(LineContainingCursor()+1, NewSection());
selectionStart = NewSelectionStart(LineContainingCursor() + 1,
"<sect1><title>");
} See how similar those two methods look? That, boys and girls , is duplication. Let s see how we might reduce it. We could write a new method, InsertTag(...), with a few string args and an integer. We might even get rid of the integer by passing in the strings in a first part / last part pair and then use the length of the first part to set the selectionStart. That looks like fun. But I think I d rather get rid of that special case code for Count == zero. I see it as the world s most radical form of duplication removal: it should exist zero times rather than just once.
I m concerned with what will happen if we just remove it. Probably some kind of exception. But in principle, why can t we just do the insert directly? One issue is the blank line between the paragraph tags. I think I ll start by removing that requirement and making the code work. Then maybe I ll put the blank line back when we re done. Step one, remove the blank line from the NewParagraph method and see what breaks:
public ArrayList NewParagraph() {
ArrayList temp = new ArrayList();
temp.Add("");
temp.Add("<P></P>");
return temp;
} changed to this:
public ArrayList NewParagraph() {
ArrayList temp = new ArrayList();
temp.Add("<P></P>");
return temp;
} A bunch of tests break. Index out of range. InsertParagraphTag has the phrase LineContainingCursor() + 2, and it should be 1 since now there s just one line being added. I ll change that to 1. Now the tests are breaking because they are wrong. For example:
[Test] public void TestOneEnter() {
model.SetLines(new String[1] {"hello world" });
model.SelectionStart = 5;
model.InsertParagraphTag();
AssertEquals(3, model.Lines.Count);
AssertEquals(18, model.SelectionStart);
} Because we ve removed the blank line, the line count should be 2 now, and the SelectionStart, let s see, I m betting 16 because we re just removing a CrLf. I ll change that; that works. All the other failures are in Customer Acceptance Tests, each showing multiple lines where there shouldn t be any. I ll change them to remove the blank lines. Here s one example:
[Test] public void StringInput() {
String commands =
@"*input
some line
*end
*enter
*output
some line
<P></P>";
InterpretCommands(commands, "");
} That blank line above the <P> tags needs to be removed. When I take it out, the test works fine. The rest of the errors are in the .test files. I ll fix them without troubling you here, unless removing blank lines isn t enough, in which case we ll be learning something. Be right back. OK, everything runs.
| Lesson | Note in passing that we ve increased the duplication between the InsertParagraphTag and InsertSectionTags methods ” they both refer now to LineContainingCursor() + 1. Sometimes making code that is nearly alike look even more alike, then removing the duplication, and then putting the differences back, is a good refactoring move. We ll see how that works out here. But right now, our mission is to remove the special cases for Lines.Count == 0. I m going to remove that clause from the following code and see what happens: public void InsertParagraphTag() { |
converting to this:
public void InsertParagraphTag() {
lines.InsertRange(LineContainingCursor()+1, NewParagraph());
selectionStart = NewSelectionStart(LineContainingCursor() + 1, "<P>");
} Three tests break: EmptyModel and the TestAllFiles in the customer tests, and TestEmptyText in TestTextModel. They are all throwing an index out-of- range exception in the InsertRange method. The message says: Index out of range. Must be non-negative and less than the size of the collection. Clearly what s happening is that the insert is getting one as the parameter and the size of the array is zero. The reason is that LineContainingCursor() returns zero if there are no lines. That s wrong, because there is no line zero. What would happen if we initialized lineNr to -1 instead of zero in the following code? I ll try it.
private int LineContainingCursor() {
int length = 0;
int lineNr = 0;
int cr = Environment.NewLine.Length;
foreach ( String s in lines) {
if (length <= selectionStart
&& selectionStart < length+s.Length + cr )
break;
length += s.Length + cr;
lineNr++;
}
return lineNr;
} Even before trying that, I was sure it wouldn t work, because if the cursor is in line zero, the foreach is going to break. I tried it anyway, and lots of things broke. That s not the fix. But TestEmptyText started to work, so it liked the -1. That at least moves the glitch to one location (removing duplication).
| Lesson | Just a tiny lesson here. Even though I was sure the change wouldn t work, I ran the tests to see. The test I was concerned about started running, although others broke. That gave me important information: that the -1 is the right answer in that case. Sometimes it s worth trying something that we know won t work for all cases, just to learn something. |
Let s make LineContainingCursor return -1 only if the array is empty. The code I tried was this:
private int LineContainingCursor() {
if (lines.Count == 0)
return -1;
int length = 0;
int lineNr = 0;
int cr = Environment.NewLine.Length;
foreach ( String s in lines) {
if (length <= selectionStart
&& selectionStart < length+s.Length + cr )
break;
length += s.Length + cr;
lineNr++;
}
return lineNr;
} But it doesn t work! The TestEmptyText still fails, as do the customer tests! But wait, I almost didn t notice that the TestEmptyText method fails in a new way. It says Expected 3 but was 12. It didn t blow up on the index out of range any more. Let s look at that test:
[Test] public void TestEmptyText() {
model.Lines = new ArrayList(new String[0]);
model.InsertParagraphTag();
AssertEquals(1, model.Lines.Count);
AssertEquals(3, model.SelectionStart);
} We have the line count right, and now it s the SelectionStart that s wrong; we re making progress. Let s look at the InsertParaTag method again and the methods it calls:
public void InsertParagraphTag() {
lines.InsertRange(LineContainingCursor()+1, NewParagraph());
selectionStart = NewSelectionStart(LineContainingCursor() + 1, "<P>");
}
private int NewSelectionStart(int cursorLine, string tags) {
return SumLineLengths(cursorLine) + tags.Length;
}
private int SumLineLengths(int cursorLine) {
int length = 0;
for (int i = 0; i < cursorLine; i++)
length += ((String)lines[i]).Length + Environment.NewLine.Length;
return length;
} I m really not sure where that 12 is coming from. One possibility is that we re somehow counting the added line twice. It contains <P></P>CrLf . Yep, that s 9, plus 3 equals 12. The loop in SumLineLengths is running once and should be running zero times. Gack! I think I see the problem, do you?
We fixed the LineContainingCursor() to return -1 if the line array is empty. After we ve done the InsertRange, it isn t empty any more and will return zero, which will cause us to count the first line. Here s the code. Notice that the highlighted second use of LineContainingCursor() will be wrong:
public void InsertParagraphTag() {
lines.InsertRange(LineContainingCursor()+1, NewParagraph());
selectionStart = NewSelectionStart( LineContainingCursor() + 1, "<P>");
} Let s buffer the LineContainingCursor() value in InsertParagraphTag:
public void InsertParagraphTag() {
int cursorLine = LineContainingCursor();
lines.InsertRange( cursorLine +1, NewParagraph());
selectionStart = NewSelectionStart( cursorLine + 1, "<P>");
} Now TestEmptyText works. I ll run all the tests...and they all run! So we ve removed the special case from InsertParagraphTag (though we ve added it to LineContainingCursor). It should be possible to remove the glitch now from InsertSectionTags and have it work as well, should it not? From
public void InsertSectionTags() {
if ( lines.Count == 0 ) {
lines.Add( " < sect1 >< title >< /title > " );
lines.Add( " < /sect1 > ");
selectionStart = 14;
return;
}
lines.InsertRange(LineContainingCursor()+1, NewSection());
selectionStart = NewSelectionStart(LineContainingCursor() + 1,
"<sect1><title>");
} to this:
public void InsertSectionTags() {
lines.InsertRange(LineContainingCursor()+1, NewSection()); selectionStart = NewSelectionStart(LineContainingCursor() + 1,
" < sect1 >< title > ");
} Arrgh, the tests don t run: that s wrong. Naturally, I forgot to buffer the cursor line in the new one, and the tests showed me. The correct code is
public void InsertSectionTags() {
int cursorLine = LineContainingCursor();
lines.InsertRange( cursorLine +1, NewSection());
selectionStart = NewSelectionStart( cursorLine + 1, "<sect1><title>");
} | Lesson | Here we see why it s so important to remove code duplication. When we have duplicate code and need to change things, we have to make the same kind of change in multiple places. When we finish this refactoring, we ll see that we wind up with only one occurrence of this buffering of LineContainingCursor. Maybe, in retrospect, we should have removed the duplication first and then the special case of Count == zero. But no harm done: let s get rid of the duplication. |
EAN: 2147483647
Pages: 291