Refactoring the C# Express Starter Kit
Yesterday I decided to take a look at the code we shipping in the Beta 1 C# Express SKU for a screen saver. I was pretty disappointed in the quality of the code, so I decided to refactor.
I’ve talked about some ideas about coding styles here on this blog, and I wanted to explore some of those ideas in this real context.
Most of my attention has been focused on the class “RssView”. 174 lines of text, 20 private fields, 13 trivial get/set properties, and only 7 methods.
Choosing a Rectangle
Let’s pick something to attack. This pair of fields are a little smelly:
// Where to draw
private Point location;
private Size size;
This notion seems familiar to me. Hmm, I think I would call it a Rectangle. So, replace it:
Rectangle location;
And start fixing up references. When I came across this reference, I had to hold my nose:
private void DrawBackground(Graphics g)
{
g.FillRectangle(new SolidBrush(BackColor), new Rectangle(Location.X + 4, Location.Y + 4, Size.Width - 8, Size.Height - 8));
g.DrawRectangle(new Pen(BorderColor, 4), new Rectangle(Location, Size));
}
What is this about? It’s trying to draw a filled, bordered rectangle. The border is 4 pixels wide. Why does it hard code the values of ‘4’ and ‘8’? Let’s make a note of this method & refactor it later.
DrawBackground
Now it’s later.
I see that Rectangle has an ‘Inflate’ method. Pushes out all 4 sides of a rectangle in exactly the way you see here. So switch to something like:
Rectangle fillRectangle = location;
fillRectangle.Inflate (-4, -4);
g.FillRectangle (new SolidBrush(BackColor), fillRectange);
The fact that we switched from Point/Size to Rectangle is what made Inflate available to us. Depending on your point of view, either:
This is proof that Rectangle really is the right way to go, and that getting the types right in your system will always make your code clearer, cleaner, and simpler.
We just got lucky, finding exactly the method we needed.
Maybe it’s the second one, but I sure seem to get lucky a lot!
BorderedRectangle
I pondered this method more, and decided that the notion of a rectangle with a border, which could draw itself, was a basic notion that I’d like to see as its own class. So I wrote one.
class BorderedRectangle
{
public class Colors
{
public readonly Color Fill;
public readonly Color Border;
public Colors(Color fill, Color border)
{
this.Fill = fill;
this.Border = border;
}
}
readonly int _fillBorderThickness;
readonly Rectangle _location;
readonly Colors _colors;
public BorderedRectangle(Rectangle location, Colors colors, int fillBorderThickness)
{
this._location = location;
this._colors = colors;
this._fillBorderThickness = fillBorderThickness;
}
public void Draw(Graphics g)
{
DrawInnerFill(g);
DrawBorder(g);
}
void DrawBorder(Graphics g)
{
Pen pen = new Pen(this._colors.Border, this._fillBorderThickness);
g.DrawRectangle(pen, this._location);
}
void DrawInnerFill(Graphics g)
{
Brush brush = new SolidBrush(this._colors.Fill);
Rectangle innerRectangle = this._location;
innerRectangle.Inflate(-this._fillBorderThickness, -this._fillBorderThickness);
g.FillRectangle(brush, innerRectangle);
}
}
There’s now a lot more lines of text to represent this simple notion. However, each line is much simpler; most of them are trivial. If there are bugs in this code, they should be easy to work out.
My first implementation had a “Color fillColor” and “Color borderColor”. I decided that these are related, so I grouped them into a new type. I think that nested types are critical to this kind of Refactoring. First, I avoid cluttering the namespace with a bunch of new tiny classes. Second, it’s clear what the relationship is between BorderedRectangle and BorderedRectangle.Classes. Finally, I can give simple names to my simple classes.
There’s a pattern here that I find myself writing a lot. A class with a series of ‘readonly’ fields, and a single public constructor that takes a parameter for each & assigns it to the field. BorderedRectangle.Colors is an example of this pattern. For lack of a better name, I’ll call it an “Entity Class”. I write it so much that I wish for a feature in the editor to help me do it. I suspect it’s a code smell, but I don’t yet see a Refactoring. Let’s keep it in mind.
I think I’m done for now, but I notice have 2 private methods, which is apparently a code smell. How would I refactor that? I think I would create a class called “Border” and a class called “Fill” and give them “Draw” methods. Maybe I’ll do that next time.
There’s still a lot of code smells in the start kit. Hopefully the pace will pick up soon.
Also, I’m working without a pair & without unit tests. I’m a little worried about that. This code wasn’t built TDD, so retrofitting tests is hard. Maybe I’ll be OK, because just running the app lets you verify it quickly. Let’s see if I get in to trouble later.