Some Starting Ideas

Our problem is that the code to define a new tag insertion is spread all over. There are seven steps, if I have remembered them all this time, and I have to edit at least two files. This is a serious indication that something is wrong. Let s think about what we would like.

Well, what I would like would be to put all the information together. To specify the strings to insert, the lines, and where the cursor goes should be at least as easy as that trick with the two strings. (See newUnorderedList and unorderedListSkip in the The Actual Work section earlier.) Since I m just dreaming, why not make it as easy as specifying a customer test? Remember the test for unordered list:

 *altU 
 *output 
 <UL> 
 <LI></LI> 
 </UL> 

I d like to have something about that easy to specify. We want to specify the menu item as well, so maybe something like this:

 Insert &Unordered List, <UL>\<LI></LI>\<UL> 
 Insert &Ordered List, <OL>\<LI></LI>\<OL> 

The backslash (\) would mean to put a new line in, and the vertical bar would represent where the cursor should go. What could we do that would be something like that, and how can we do it without much investment?

Lesson  

Doing it without much investment is important for two reasons. First of all, our mission is always to deliver real value to our customer. We need to be careful when we make improvements like this that they really do pay off for the customer. If we can make the system better with a very small investment, that s a good thing. Second, it s important to the premise of this book that it s practical to design and build software incrementally, with the confidence that we won t get into a situation where it s a big deal to get back out again. If this doesn t work, I have to cancel the book!

Now all the methods and steps we have to go through have a purpose that is essentially simple: at the end of it all, a given menu click wants to call InsertTags() with the correct two string arrays to do the insert. What if we had a kind of MenuItem that knew the two string arrays, and its EventHandler just passed them directly to the model? Might everything else collapse out? Let s try it.

I ll begin by creating a subclass of MenuItem called NotepadMenuItem. I ll give it a constructor that accepts the two string arrays we need, and I ll use it for one of our menus . Here goes:

 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 string[] Inserts { 
 get { return tagsToInsert; } 
 } 
 public string[] Skips { 
 get { return tagsToSkip; } 
 } 
 } 

You may have noticed that I implemented get properties for the tags. I did that on speculation, which is a bit against my preferred practice, but I was on a roll and just couldn t stop myself . We ll see whether it causes problems, but I don t expect that it will. Now I m going to use the new NotepadMenuItem in just one place. I ll change this:

 ... 
 insertSection = new MenuItem ( 
 "Insert &Section", 
 new EventHandler(MenuInsertSection)); 
 insertPre = new MenuItem ( 
 "Insert &Pre", 
 new EventHandler(MenuInsertPre)); 
 insertUnorderedList = new MenuItem ( 
 "Insert &UL", 
 new EventHandler(MenuInsertUnorderedList)); 
 this.Menu = new MainMenu(new MenuItem[] {fileMenu, insertPre, 
 insertSection, insertUnorderedList} ); 
 ... 

so that I get this:

 insertSection = new MenuItem ( 
 "Insert &Section", 
 new EventHandler(MenuInsertSection)); 
 insertPre = new NotepadMenuItem ( 
 "Insert &Pre", 
 new EventHandler(MenuInsertPre), 
 { "<pre></pre>" }, 
 { "<pre>" } ); 
 insertUnorderedList = new MenuItem ( 
 "Insert &UL", 
 new EventHandler(MenuInsertUnorderedList)); 
 this.Menu = new MainMenu(new MenuItem[] {fileMenu, insertPre, 
 insertSection,  insertUnorderedList} ); 

Unfortunately, that doesn t compile. Apparently we can t use that array of string literal notation quite that freely . Oh, we have to create the array with new, like this:

 insertSection = new MenuItem ( 
 "Insert &Section", 
 new EventHandler(MenuInsertSection)); 
  insertPre = new NotepadMenuItem (  
  "Insert  &  Pre",  
  new EventHandler(MenuInsertPre),  
  new string[] { "  <  pre  ><  /pre  >  " },  
  new string[] { "  <  pre  >  " } );  
 insertUnorderedList = new MenuItem ( 
 "Insert &UL", 
 new EventHandler(MenuInsertUnorderedList)); 
 this.Menu = new MainMenu(new MenuItem[] {fileMenu, insertPre, 
 insertSection, insertUnorderedList} ); 

This compiles. I m sure it will run, but let s run the tests...this is embarrassing! A test broke, and it doesn t have anything to do with this change! Somehow I haven t been running all the tests. The error arises in two of the CustomerTests. One is EmptyModel, which looks like this:

 [Test] public void EmptyModel() { 
 form.XMLKeyDownHandler((object) this, 
 new KeyEventArgs(Keys.Enter)); 
 AssertEquals("<P></P>\r\n", model.TestText); 
 } 

The error message refers to an invalid or negative subscript in the method InListItem(), which we just implemented. That code looks like this:

 private Boolean InListItem() { 
 return ((string) lines[LineContainingCursor()]).StartsWith("<LI>"); 
 } 

The bug is clear: If the input is empty, LineContainingCursor() will return -1 and we will try to get that line. And there is no such line to get, causing the exception. This should fix it:

 private Boolean InListItem() { 
  if (LineContainingCursor()  <  0 ) return false;  
 return ((string) lines[LineContainingCursor()]).StartsWith("<LI>"); 
 } 

And it does. I must have been running some subset of the tests when I put that last feature in. Very bad.

Lesson  

Well, as they say, Even Homer sometimes nods. I believe they are referring to the Greek Homer, not the yellow television character. We will make mistakes operationally, and I just did. Try not to emulate this feature of your humble author: be sure to run all your tests between tasks . I have often advised people to run the tests before starting some new phase, and I didn t follow my own advice. It caused some confusion there for a minute.

However, no harm done. The new NotepadMenuItem works perfectly ” and why wouldn t it? We aren t using any of its special features yet. Now we ll do that. The EventHandler for that menu item is MenuInsertPre:

 void MenuInsertPre(object obj, EventArgs ea) { 
 CallModel(insertPreTagAction); 
 } 

I believe that the object obj parameter to a menu click is the MenuItem itself. I ll test that quickly by trying to print the Inserts array:

 void MenuInsertPre(object obj, EventArgs ea) { 
 NotepadMenuItem item = (NotepadMenuItem) obj; 
 Console.WriteLine("Inserts[0] = {0}", item.Inserts[0]); 
 CallModel(insertPreTagAction); 
 } 

Sure enough, in the Standard Out window of NUnit, I see a couple of lines verifying that we got what we wanted:

 Inserts[0] = <pre></pre> 
 Inserts[0] = <pre></pre> 

Perfect. Now let s see if we can change the MenuInsertPre method to call InsertTags() directly. Remember that the CallModel() method is there to send the text from the TextBox over to the TextModel and to get it back:

 private void CallModel(ModelAction modelAction) { 
 GetText(); 
 modelAction(); 
 PutText(textbox, model.LinesArray(), model.SelectionStart); 
 } 

We ll just move the GetText() and PutText() into our MenuInsertPre method for now and call the InsertTags directly:

 void MenuInsertPre(object obj, EventArgs ea) { 
 NotepadMenuItem item = (NotepadMenuItem) obj; 
 GetText(); 
 model.InsertTags(item.Inserts, item.Skips); 
 PutText(textbox, model.LinesArray(), model.SelectionStart); 
 } 

InsertTags() isn t public in TextModel, so I ll promote it:

 public void InsertTags(string[] tagsToInsert, string[] tagsPrecedingCursor) { 
 int cursorLine = LineContainingCursor(); 
 lines.InsertRange(cursorLine+1, tagsToInsert); 

We should be good to go. Let s compile and test. The code compiles, and the test runs! This is good. We can now do the following steps:

1. Rename the MenuInsertPre() handler to reflect its more general purpose, probably to MenuInsertTags().

2. Change all the menu items that insert tags to use MenuInsertTags().

3. Remove all the excess code that we no longer need. This will include the handlers in XMLNotepad.cs, the ModelActions, and the string array definitions in TextModel.

Follow me through the steps. When we re finished we ll take a look at the overall results. Here s the renaming of the handler:

 void  MenuInsertTags  (object obj, EventArgs ea) { 
 NotepadMenuItem item = (NotepadMenuItem) obj; 
 GetText(); 
 model.InsertTags(item.Inserts, item.Skips); 
 PutText(textbox, model.LinesArray(), model.SelectionStart); 
 } 
 
 insertPre = new NotepadMenuItem ( 
 "Insert &Pre", 
 new EventHandler(  MenuInsertTags  ), 
 new string[] { "<pre></pre>" }, 
 new string[] { "<pre>" } ); 

And here I m changing the other menus to use NotepadMenuItem:

  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),  
  new string[] {"  <  UL  >  ","  <  LI  ><  /LI  >  ","  <  /UL  >  "},  
  new string[] { "  <  UL  >  ", "  <  LI  >  " } );  
 insertUnorderedList = new NotepadMenuItem ( 
 "Insert &UL", 
 new EventHandler(MenuInsertTags), 
 new string[] { "<pre></pre>" }, 
 new string[] { "<pre>" } ); 
 this.Menu = new MainMenu(new MenuItem[] {fileMenu, insertPre, 
 insertSection, insertUnorderedList} ); 

Oops! Tests don t run. Do you see what I did? I changed the Pre menu to insert the UL data. The tests saved me, which is what they are for. The corrected code is

 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), 
 new string[] { "<pre></pre>" }, 
 new string[] { "<pre>" } ); 
 insertUnorderedList = new NotepadMenuItem ( 
 "Insert &UL", 
 new EventHandler(MenuInsertTags), 
 new string[] {"<UL>","<LI></LI>","</UL>"}, 
 new string[] { "<UL>", "<LI>" } ); 

The tests are all running, and the menus are all going through the MenuInsertTags method. I should be able to delete lots of code. I ll begin by commenting out everything that I would like to delete.

XMLNotepad.cs

 class XMLNotepad : Form { 
 public TestableTextBox textbox; 
 private TextModel model; 
 private MenuItem insertPre; 
 private MenuItem insertSection; 
 private MenuItem insertUnorderedList; 
 private MenuItem openFile; 
 private MenuItem saveFile; 
 private ModelAction enterAction; 
 private ModelAction shiftEnterAction; 
 //    private ModelAction insertSectionAction; 
 //    private ModelAction insertPreTagAction; 
 //    private ModelAction insertUnorderedListAction; 
 private ModelAction saveAction; 
 private ModelAction loadAction; 
 private String fileName; 
 private Boolean displayDialog = true; 
 public delegate void ModelAction(); 
 ... 
 private void InitializeDelegates(TextModel model) { 
 enterAction = new ModelAction(model.Enter); 
 shiftEnterAction = new ModelAction(model.InsertReturn); 
 //      insertSectionAction = new ModelAction(model.InsertSectionTags); 
 //      insertPreTagAction = new ModelAction(model.InsertPreTag); 
 //      insertUnorderedListAction = new ModelAction(model.InsertUnorderedList); 
 saveAction = new ModelAction(this.SaveFile); 
 loadAction = new ModelAction(this.LoadFile); 
 } 
 //    void MenuInsertUnorderedList(object obj, EventArgs ea) { 
 //      CallModel(insertUnorderedListAction); 
 //    } 
 // 
 //    void MenuInsertSection(object obj, EventArgs ea) { 
 //      CallModel(insertSectionAction); 
 //    } 
 ... 
 } 

With these items removed, the system still compiles and the tests run. I ll complete the deletion of these commented-out items. Normally I would delete these lines in one step. Even though I commented them out for your reading convenience only, now I m tempted to leave them in just in case. Because all the code is backed up in the code manager, there s no reason to confuse ourselves with dead code. The program still compiles, and the tests run correctly. Now I d like to remove some code from TextModel. I ll show you what I want to do:

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

It seems to me that I should be able to remove all this code. However, this time the compiler complains. It seems that tests in TestTextModel.cs are referring to some of these methods. For example:

 [Test] public void AltS() { 
 model.Lines = new ArrayList(new String[0]); 
 model.AltS(); 
 AssertEquals("<sect1><title></title>", model.Lines[0]); 
 AssertEquals("</sect1>", model.Lines[1]); 
 AssertEquals(14, model.SelectionStart); 
 } 

This is a perfectly good test. We ve changed the program such that the model can change tags by using its general InsertTags() method but the more specific methods, such as the now oddly-named AltS(), will no longer be hooked up. Similarly, there are tests we wrote for InsertPreTag() and InsertUnorderedList(), and perhaps others. It s easy enough to change these tests to use InsertTags(), and I expect that we ll do that, but look what happens when I change the AltS test:

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

(There is another test, AltSWithText(), that is changed similarly. This one is enough to make my point.) When we make this change, this test runs correctly. However, it s testing less than it did before. In the old form, the test ensured that the strings defining AltS were correctly set up inside TextModel class. Now ”since we have deleted the strings ”all this test does is assure us that if the strings are set up like they are in the test, the insertion will work. The tests are weakened by our refactoring.

Now, it s no surprise that tests often need changing when we refactor the code, especially when, as this time, we are refactoring code across two classes ”in our case, XMLNotepad and TextModel. But we don t like to see the tests getting weaker. And they are exposing a real problem, or at least a potential one.

As it happens, the customer tests go all the way through the menu items, so when a customer test calls for an AltS, we know that it is testing the actual string needed in the InsertSectionTags concept. We could enhance these tests to go through the menu, but that would be redundant, and again, it would weaken them. Right now, these tests test the model directly, and we like that. If we put them through the form, they ll be less direct, and if one of them ever breaks, we won t be sure if the problem is in the form, or in the model. The code is trying to tell us something. What is it?

Lesson  

Duplication. So often it s duplication that gives us the signal. Here we have duplication between the Form and the tests, regarding the strings to be inserted. We want to insert the sequence for a <pre> tag ”what object should know how to do that? Surely not both the Form and the tests. More likely, it s the TextModel, because it s the TextModel s job to know how to format XML. The duplication was the signal.