Improving the Platform

We re on a mission to build features, so we can t spend too much time. But we have a responsibility to produce clean code, and this isn t it. My suggestion is that we change the skip tags to be like the insert tags ”an array. Then we ll make the count calculation a little stronger than just .Length . Since I have all those tests, I m just going to change all the skip tags to be arrays and then make the tests run. Here are the tag changes:

 class TextModel { 
 private static string[] newParagraph = { "<P></P>" }; 
 private static string  []  paragraphSkip =  { "  <  P  >  " };  
 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  >  " };  
 private static string[] emptyLine = { "" }; 
 private static string  []  emptyLineSkip =  { "" };  
 ... 

This won t compile, of course. I ll compile to find out why, so I won t have to guess. Right, InsertTags expects a scalar and we want to give it an array:

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

We can see that we ll have to change NewSelectionStart as well. It looks like this:

 private int NewSelectionStart(int cursorLine, string tags) { 
 return FirstPositionOfLine(cursorLine) + tags.Length; 
 } 

I ll just change the parameter to be an array and posit a new method to return the length, like this:

 private void InsertTags(string[] tagsToInsert,  string[]  tagsPrecedingCursor) { 
 int cursorLine = LineContainingCursor(); 
 lines.InsertRange(cursorLine+1, tagsToInsert); 
 selectionStart = NewSelectionStart(cursorLine + 1, tagsPrecedingCursor); 
 } 
 private int NewSelectionStart(int cursorLine,  string[]  tags) { 
 return FirstPositionOfLine(cursorLine) +  TotalTagLength(tags);  
 } 

That compiles, as expected, with just the message about TotalTagLength not being defined.

Lesson  

You might have written TotalTagLength before compiling, and on another day so might I. Today, I decided to let the compiler tell me to do it. I m trying to use my brain as little as possible so that it won t wear out.

Now for TotalTagLength:

 private int TotalTagLength(string[] tags) { 
 int result = (tags.Length -1) * Environment.NewLine.Length; 
 foreach (string tag in tags) { 
 result += tag.Length; 
 } 
 return result; 
 } 

Oddly, or perhaps not oddly, all the tests run except my new one. Now it s getting 17 instead of 19. What s up with that? Oh! Look at the unorderedListSkip definition earlier. I left the angle brackets off the LI. The line should be

 private static string[] unorderedListSkip = { "<UL>", "<  LI  >" }; 

Tests all run again. The world is a bit better place. Shall we return to implementing, or should we do a little more cleanup? Let s scan the code a bit to decide. There is duplication in those insert and skip literals: similar strings appear in each. It s almost tempting to use the trick with the vertical bar that we used in the customer tests. I m held back because I remember that part of our motivation for going to the arrays of strings was that we didn t like the way the multiline strings looked in the code. The multiple lines look OK in the test files, but in the source they would have to be left-justified and they would make the code hard to read.

I notice some duplication in these methods :

 public void InsertParagraphTag() { 
 InsertTags(newParagraph, paragraphSkip); 
 } 
 
 public void InsertPreTag() { 
 InsertTags(newPre, preSkip); 
 } 
 public void InsertSectionTags() { 
 InsertTags(newSection, sectionSkip); 
 } 
 public void InsertUnorderedList() { 
 InsertTags(newUnorderedList, unorderedListSkip); 
 } 

We have a number of methods that are each calling InsertTags, the only difference being the parameters. Each of those methods is called by a different menu action. We might do something more sophisticated there, but I think it s best not to. The Form should know as little as possible about how the model is going to do things, so moving knowledge of the model over to the Form wouldn t be a good idea. We might consider passing the menu item itself, or some parameter that was attached to the menu item, over to the TextModel to identify it. That doesn t seem quite right.

One possibility might be to have a single TextModel operation, pretend it s called TagInsertion for now, and pass it a parameter saying what kind of tags to insert, something like this:

 private void InitializeDelegates(TextModel model) { 
 enterAction = new ModelAction(model.Enter); 
 shiftEnterAction = new ModelAction(model.InsertReturn); 
 insertSectionAction = new ModelAction(model.  TagInsertion("section");  
 insertPreTagAction = new ModelAction(model.  TagInsertion("pre");  
 saveAction = new ModelAction(this.SaveFile); 
 loadAction = new ModelAction(this.LoadFile); 
 } 

Then we could look up the tags to insert and to skip over in the TextModel. My intuition is that this will make the code more complex at this point, and so I m going to hold off on that. The end result is that we have considered some possible directions for code improvement, but we re not going to do any. I think it s time for the menu.