Refactoring the XMLNotepad
I’ve been reading Extreme Programming Adventures in C#. Currently reading Chapter 28 (Undo).
Through most of the book, there has been a bit of Refactoring that the code has been crying out for. At first I thought Ron was waiting to until the duplication got worse, thereby justifying the Refactoring as providing real customer value.
I finally downloaded the final sources from the MSPress site, and see that the smells are still there. So I did the Refactorings myself, and found that the code is much improved for it.
Duplication between SkipTags and Insert Tags:
private static InsertAction[] insertActions = new InsertAction[] {
new InsertAction(
Tags.Pre,
"Insert &Pre",
new string[] { "<pre></pre>" }, // <------ here
new string[] { "<pre>" }), // <------ here
I changed it to take an array of tags:
new string[] { "pre" }),
And figure out the insert & skip text later:
public string[] TagsToInsert
{
get
{
List<string> list = new List<string>();
list.AddRange(TagsToSkip);
List<string> reversed = new List<string>(tags); reversed.Reverse();
foreach (string s in reversed)
{
list.Add("</" + s + ">");
}
return list.ToArray();
}
}
public string[] TagsToSkip
{
get
{
List<string> list = new List<string>();
foreach (string s in tags)
{
list.Add("<" + s + ">");
}
return list.ToArray();
}
}
This is not, strictly speaking, a Refactoring. The original code could insert tags on the same line, and my code always puts new tags on new lines. I should probably talk to my customer to understand how important this is to them. If it is important, I’d need to do enough additional work that the Refactoring might be too expensive and should be abandoned.
Still, I like the effect on the code, so I’m going to try to get my customer to go along.
Note: I’m cheating by using C# 2.0 features like List<>. Ron didn’t have them when writing his book.
Duplication around the ‘Tags’ enum
There’s a lot of it. There’s a bunch mapping back and forth between the enum & the InsertAction command. I decided to make them the same:
public static class InsertActions
{
public static InsertAction Pre = new InsertAction(
"Insert &Pre",
new string[] { "pre" }
);
public static InsertAction Section = new InsertAction(
"Insert &Section",
new string[] { "sect1", "title" }
);
public static InsertAction UnorderedList = new InsertAction(
"Insert &UL",
new string[] { "UL", "LI" }
);
public static InsertAction OrderedList = new InsertAction(
"Insert &OL",
new string[] { "OL", "LI" }
);
public static InsertAction Paragraph = new InsertAction(
null,
new string[] { "P" }
);
public static InsertAction ListItem = new InsertAction(
null,
new string[] { "LI" }
);
//
// it's important that all new InsertActions be added to this list, or they
// won't get handled correctly. That means there's a little duplication still,
// but it's very much localized now. Yay OO.
//
public static InsertAction[] All
{
get
{
return new InsertAction[] {
Pre,
Section,
UnorderedList,
OrderedList,
Paragraph,
ListItem
};
}
}
}
A lot of the associated changes are quite simple:
public void Enter()
{
if (InListItem())
InsertTags(InsertActions.ListItem);
else
InsertTags(InsertActions.Paragraph);
}
We also get rid of a bit of code. SectionCommand() is replaced with InsertActions.Section, which is a huge win in removing duplication.
public InsertAction SectionCommand() {
foreach (InsertAction a in InsertActions) {
if (a.MenuString == "Insert &Section" ) return a;
}
return null;
}
And InsertActionForCommand is gone:
private InsertAction ActionForCommand(Tags command) {
foreach ( InsertAction action in InsertActions) {
if (action.Command == command)
return action;
}
return null;
}
Now that it’s done, now how much simpler the creation of InsertAction is.
Before
new InsertAction(
Tags.Pre,
"Insert &Pre",
new string[] { "<pre></pre>" }, // <------ here
new string[] { "<pre>" }), // <------ here
After
public static InsertAction Pre = new InsertAction(
"Insert &Pre",
new string[] { "pre" }
);
I think this is a real improvement.