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.
Comments
- Anonymous
July 20, 2004
What if you added an attribute to each InsertAction you created and then built the All list using reflection? That would be one way to eliminate the duplication. - Anonymous
July 20, 2004
Nice changes! Why weren't you there to pair with me? ;-> - Anonymous
July 20, 2004
Nicholas: I might do that in my own code, in my current job.
However, I wouldn't do that in the book, because it doesn't fit with the principles of the book. In particular, using Reflection in this way is confusing to me (because I'm not familiar with it), and because it seems like overkill for the problem (the unknown often seems like overkill).
Following Ron's lead, if you & I were pairing & you suggested Reflection, I probably let you explain/show it to me. So, why not write up an example for us? - Anonymous
July 20, 2004
Jay, I agree it's overkill for an example in a book like this (although it would fit in well if you had a solid chapter on reflection). I was trying to brainstorm a little for how far something like "eliminate duplication" could be taken. I'll try to write up something later if I have the time. - Anonymous
July 20, 2004
The comment has been removed - Anonymous
July 20, 2004
Cyrus (http://blogs.msdn.com/cyrusn/) suggested an intesting trick: have each of the singleton objects register themselves with the collection.
1. Change the 'All' property to:
public static List<InsertAction> All = new List<InsertAction>();
2. In the InsertAction ctor, add:
TextModel.InsertActions.All.Add(this);
Pretty simple, and it works. Thanks Cyrus. - Anonymous
July 20, 2004
I like the trick from Cyrus. It's concise and you can add actions without thinking too much. We still have to be a little careful because now it's possible for the All list to include actions not present in the enumeration.
Is it ok for the list of actions and the enumeration to be mutable? Again, something for thinking about rather than the book. I wonder if untrusted code could inject an action and gain some priviledges or information. - Anonymous
July 21, 2004
The mutability question is a good one.
In my implementation, I marked all the "enum" members as 'readonly', so we get most of the way there.
There's some broken encapsulation with the 'All' array as presented. I have some ideas for how to improve it, but they seemed to be bigger steps than I wanted to take at the time.
I think the code is telling us that the code that the part about "TextModel.InsertActions" and "class InsertAction" are closely coupled, but that coupling isn't properly respected in the code.
I would start by bringing them closer together, possibly with some nested classes or a factory or something.
There's another refactoring that I'd like to explore, which also Cyrus suggested. Instead of 6 instances of 1 class, have 6 derived classes which pass these parameters to the base ctor. I think it might clean up the code further.
In the book context, though, I'd want to stop here & let it go for now. But you know that. - Anonymous
June 09, 2009
PingBack from http://menopausereliefsite.info/story.php?id=1499