Removing the Duplication
We know that we want the TextModel to be the only object that knows how to do these things. We have tried pushing in that direction before, by using method names like InsertPreTag, and with AltS, but this led us to a very complex structure in the Form. We like the way the form works now: we just specify a menu item and that item includes enough information to do the job. But in a sense, the NotepadMenuItem has too much information. It knows not what to do, but how to do it. Let s change it to know what, not how. Instead of giving the TextModel a string, we ll give it a command.
One possibility is to use a string, like pre or Insert Pre or even Insert &Pre , as the command. This leaves open the chance that we would say pork or Insrot Per and not find out until a test failed or the program breaks. It s better for us if we use commands that can be checked by the compiler, as were the methods in our earlier implementation. I m thinking that we need a simple list of possible commands, perhaps an Enumeration.
| Lesson | One of my technical editors objects strongly to what follows . To him, the use of an Enum is an across-the-room code smell. He believes that a class would have been a better choice, and based on a few shared e- mails with him, I m sure he s right. Using a class would let us encapsulate more of the literal constants and would let us move some additional logic inside the resulting new class. Unfortunately for all of us, the history of the project never gave us that discovery. So be warned that, as in other cases in this program, a better way surely exists. |
It would be possible to work all this out before doing it. Instead, I m going to work through it bit by bit, letting the compiler and tests help me. I m starting from a green bar, with the XMLNotepad converted to use the new NotepadMenuItem and the TextModel looking like this:
class TextModel {
private ArrayList lines;
private int selectionStart;
private static string[] newParagraph = { "<P></P>" };
private static string[] paragraphSkip = { "<P>" };
private static string[] newListItem = { "<LI></LI>" };
private static string[] listItemSkip = { "<LI>" };
private static string[] emptyLine = { "" };
private static string[] emptyLineSkip = { "" };
// private static string[] newSection = {"<sect1><title></title>",
// "</sect1>" };
// private static string[] sectionSkip = { "<sect1><title>" };
private static string[] newUnorderedList = {"<UL>","<LI></LI>","</UL>"};
private static string[] unorderedListSkip = { "<UL>", "<LI>" };
private static string[] newPre = { "<pre></pre>" };
private static string[] preSkip = { "<pre>" };
public void InsertPreTag() {
InsertTags(newPre, preSkip);
}
// public void InsertSectionTags() {
// InsertTags(newSection, sectionSkip);
// }
public void InsertUnorderedList() {
InsertTags(newUnorderedList, unorderedListSkip);
}
// public void AltS() {
// InsertSectionTags();
// }
// public void AltP() {
// InsertPreTag();
// }
... I ve put the InsertPreTag() and InsertUnorderedList() methods back in. The AltS() and AltP() methods aren t needed, because I ve left the modified tests in, looking like this:
[Test] public void AltS() {
model.Lines = new ArrayList(new String[0]);
model.InsertTags(
new string[] {"<sect1><title></title>","</sect1>" },
new string[] { "<sect1><title>" });
AssertEquals("<sect1><title></title>", model.Lines[0]);
AssertEquals("</sect1>", model.Lines[1]);
AssertEquals(14, model.SelectionStart);
} Should we back up to a better place, with the the TextModel and tests unmodified, before we started the changes to the TextModel? You might argue that it would be a more stable point to work from. On the other hand, with some tests modified to use the new InsertTags feature, we know that we ll get an immediate look at how our new command idea impacts the tests. It s a judgment call. We ll start from here.
First, let s add an enum. We ll add it inside TextModel:
public enum Tags {
Pre = 1,
Section = 2,
UnorderedList = 3
} Now we want to modify the NotepadMenuItem to use the enum instead of the strings that it uses currently. Remember that the code looks like this:
insertPre = new NotepadMenuItem (
"Insert &Pre",
new EventHandler(MenuInsertTags),
new string[] { "<pre></pre>" },
new string[] { "<pre>" } );
...
void MenuInsertTags(object obj, EventArgs ea) {
NotepadMenuItem item = (NotepadMenuItem) obj;
GetText();
model.InsertTags(item.Inserts, item.Skips);
PutText(textbox, model.LinesArray(), model.SelectionStart);
}
We want to have it look more like this:
insertPre = new NotepadMenuItem (
"Insert &Pre",
new EventHandler(MenuInsertTags),
TextModel.Tags.Pre );
...
void MenuInsertTags(object obj, EventArgs ea) {
NotepadMenuItem item = (NotepadMenuItem) obj;
GetText();
model.InsertTags( item.Command );
PutText(textbox, model.LinesArray(), model.SelectionStart);
}
Then, inside TextModel, we ll have to look up the various strings needed. This feels like a big step to me. I can see two ways to do it. The long way will be to do the above, enhancing TextModel to deal with the command to make the tests run. We might be able to get the tests running a bit sooner if we look up the strings on the Form side, as part of the MenuInsertTags method, and pass them to InsertTags(). That will get the tests running again. Then we would change the InsertTags() signature, by using the lookup already implemented, moving to green bar a second time.
Either way would work. I m tempted to try it in one step, and I m pretty sure I can make it work. Let s do it in the smaller steps anyway, just as practice in baby steps.
In fact, as soon as I get the baby steps idea, I see an even better plan! I ll look up the two strings in the NotepadMenuItem constructor, not in the MenuInsertTags method as I was first thinking. Like this:
class NotepadMenuItem : MenuItem {
private string[] tagsToInsert;
private string[] tagsToSkip;
public NotepadMenuItem
(String menuString, EventHandler handler, string[] inserts, string[] skips)
:base(menuString, handler) {
tagsToInsert = inserts;
tagsToSkip = skips;
}
public NotepadMenuItem (String menuString, EventHandler handler,
TextModel.Tags tags)
:base(menuString, handler){
tagsToInsert = TextModel.InsertString(tags);
tagsToSkip = TextModel.SkipString(tags);
}
public string[] Inserts {
get { return tagsToInsert; }
}
public string[] Skips {
get { return tagsToSkip; }
}
} Get the idea? I ll just create the menu item to grab the strings from the TextModel and plug them in. That only requires me to build the InsertString and SkipString methods on TextModel, and then the tests should run:
public static string[] InsertString(Tags tag) {
return newPre;
}
public static string[] SkipString(Tags tag) {
return preSkip;
} For now, I ll just return this one value and then change only the Pre menu:
insertSection = new NotepadMenuItem (
"Insert &Section",
new EventHandler(MenuInsertTags),
new string[] {"<sect1><title></title>","</sect1>" },
new string[] { "<sect1><title>" } );
insertPre = new NotepadMenuItem (
"Insert &Pre",
new EventHandler(MenuInsertTags),
TextModel.Tags.Pre );
insertUnorderedList = new NotepadMenuItem (
"Insert &UL",
new EventHandler(MenuInsertTags),
new string[] {"<UL>","<LI></LI>","</UL>"},
new string[] { "<UL>", "<LI>" } );
We see how this is shaping up. It looks a bit better to me already. With the current stubbed implementation of InsertString() and SkipString(), it should work. I notice in passing that the methods return arrays of string, so they should be plural, not singular. We ll fix that in a moment. We need to be sure we re on a green bar first...and running the tests shows that we are. The code runs. We ll rename those methods now, using the refactoring tool. Then we ll modify one of the other menus , to get a red bar.
"Insert &Section",
new EventHandler(MenuInsertTags),
TextModel.Tags.Section );
This breaks a customer test, with this output:
c:\data\csharp\notepad\sect1.test
*Expected
<sect1><title></title>
</sect1>
*Result
<pre></pre>
I would be happier if it broke a unit test also, but I didn t change any of them yet. We could do it now or wait. I m inclined to wait. My plan is to remove the constructor for NotepadMenuItem that accepts the two arrays, and when we do that, we ll get compiler messages for the tests that need changing. We ll let the computer tell us what do to. To make the current test run, we need to improve the lookups:
public static string[] InsertStrings(Tags tag) {
if (tag == Tags.Pre) return newPre;
else if (tag == Tags.Section) return newSection;
else return newParagraph;
}
public static string[] SkipStrings(Tags tag) {
if (tag == Tags.Pre) return preSkip;
else if (tag == Tags.Section) return sectionSkip;
else return paragraphSkip;
} To make this work, I had to uncomment the newSection and sectionSkip lines shown following the next Lesson. Note that I also needed to have an else clause. I decided to return the paragraph insert information. An alternative might have been to return null or to throw an exception. We re not finished with this yet, so for now this should work.
| Lesson | There is a potential problem with this practice: we might forget and leave the code in this awkward state. I haven t looked forward to see whether I got away with it this time, but if you think this is a bad idea, you may be right. The tradeoff is this, however: if we fill in too many of these gaps, it will take us longer to get from test red to test green, and we ll have to think about too many things at once. That will slow us down, but this practice could leave a bug in the system. You might find it valuable to put a TODO in the code, or write a note on a card, or write another test. The main thing is to remain sensitive to what you re doing, and to adjust your practices as you notice problems. private static string[] newParagraph = { "<P></P>" }; |
The tests do run. Let s do it again with the remaining menu:
"Insert &UL",
new EventHandler(MenuInsertTags),
TextModel.Tags.UnorderedList );
This, of course, breaks the UnorderedList test, and we fix it with
public static string[] InsertStrings(Tags tag) {
if (tag == Tags.Pre) return newPre;
else if (tag == Tags.Section) return newSection;
else if (tag == Tags.UnorderedList) return newUnorderedList;
else return newParagraph;
}
public static string[] SkipStrings(Tags tag) {
if (tag == Tags.Pre) return preSkip;
else if (tag == Tags.Section) return sectionSkip;
else if (tag == Tags.UnorderedList) return unorderedListSkip;
else return paragraphSkip;
} Now we can remove the original NotepadMenuItem constructor, deleting this code:
public NotepadMenuItem
(String menuString, EventHandler handler, string[] inserts, string[] skips)
:base(menuString, handler) {
tagsToInsert = inserts;
tagsToSkip = skips;
}
| Lesson | Try to get the sense of what we re doing here. We are going in tiny steps, one simple change at a time. Each time, either the compiler tells us what to do next or the tests tell us. We change a couple of lines, and everything works again. We re never more than a few moments from a correctly working program. When you get experienced at working this way, you ll probably notice a couple of things. First, you ll rarely make a serious mistake that has to be backed out. Each step goes forward, rarely back. Second, you ll find that you have a low level of stress as you work. Get used to this feeling, and use the appearance of stress as a clue that your steps may be too big. My strong advice is to try working like this until you can find these tiny steps. You won t always work that way ”you ve seen in this book that I don t always work that way ”but you will find, I think, that you re working in smaller and more secure steps than ever before. Remember also how often I get into trouble when I take bigger steps. Learn from the master : don t do as he does. |
EAN: 2147483647
Pages: 291