Refactoring the C# Express Starter Kit – Part 2: Organizing fields
Looking in RssView, I see a big mess. There are lots of fields. Some are related to each other & different from others, which suggest an Extract Class.
Some are set at initialization time, while others change over time. Clearly there’s a difference there, but it’s not called out in the code.
All the initialize-time fields are set via public properties after the object is created, but should be set via the constructor. As it is, there’s a risk of not setting a required property, or of a property changing value later when that’s a bad idea. Also, it’s valuable to mark fields ‘readonly’ when possible. There seems to be a split between “identity” and “state” fields. I want to explore the value of putting all the initialize-time fields in a helper class, and all the state fields in another. I want to see what happens when I do that. Also, I’m thinking of Ron Jeffries suggestion that fields should all change at the same rate.
Since most of the fields are initialize-time, I’m going to start there first.
I take all the fields that look like init-time, and wrap them in a nested class:
public class Configuration_
{
// Where to draw
public Rectangle Location;
//...
}
Configuration_ configuration = new Configuration_();
public Configuration_ Configuration { get { return this.Configuration; } }
I have the underscore because otherwise there would be a conflict between the name of the property and the name of the type. I hope to eventually figure out a better way to name these things, without the underscore, but I don’t want my inability to think of a good name to stop my Refactoring fun.
Next I go to all the references to those fields and add “Configuration.”:
this.Configuration.rssChannel = rssChannel;
I run, and it immediately crashes. The ‘Configuration’ property has an infinite recursion. This is a common coding error, at least when I’m doing the coding. If I was pairing, maybe my pair would have noticed. If I had a unit test, maybe it would have caught this sooner. If I was following my naming convention of prefixing private fields with an underscore, maybe I would have caught this sooner. If the compiler would warn on this, that would be nice.
Luckily it’s easy to fix. I run again, and things seem to be working.
I see that the top of the screen has a white box across it. I can’t remember if that’s supposed to be there or not. I should go back to the original version of the sources & see what happens there, but I’m feeling lazy and/or rushed.
Oh yeah, I should make the ‘configuration’ field readonly, since I can.
The last thing I want to do before I go is to get rid of the properties & use public readonly fields. You already know that I don’t like trivial properties. I get the same syntax at 1/3 the code.
Search/replace and rectangle select are pretty useful for this, but I wish the IDE would make it easier on me.
In the process I discover an issue with the code. The max & min items to show are set in two places. They have a default initialized value, and a passed-in value. I think that Refactoring to use the readonly fields pattern has helped make this issue more visible.
I’m not sure who should own the value; for the time being, I’ll make them ‘const’ instead of ‘readonly’ and let the RssView own them.
The Configuration_ class now looks like this:
public class Configuration_
{
// Where to draw
public readonly Rectangle Location;
//...
public Configuration_(Rectangle location, RssChannel rssChannel, Color backcolor, Color bordercolor, Color forecolor, Color titlebackcolor, Color titleforecolor, Color selectedforecolor, Color selectedbackcolor)
{
this.Location = location;
//...
}
}
The construction of the RssView now looks like:
RssView.Configuration_ config = new RssView.Configuration_(
new Rectangle(new Point(2 * Size.Width / 20, 3 * Size.Height / 20), new Size(10 * Size.Width / 20, 12 * Size.Height / 20)),
//...
);
// Initialize the two classes that draw the RSS contents
// Use the first channel of the RSS feed
rssView = new RssView(config);
Now the RssView only has 5 fields; we’re making some kind of progress.
I’d like to refactor all those ‘Color’ values. There seem to be relationships between them. I imagine that a class that represents a foreground/background color pair would be valuable.
I’m also confused by that huge expression that selects the location of the RssView. What does it all mean? Why does it multiply by 2 & divide by 20? Is that different than dividing by 10? If I can figure out what it’s trying to do, maybe I can find a clearer way of expressing it.
In general, I think that having data (the color values) in my code is a bad idea. It makes unit testing more complicated. It makes seeing the logic in the code harder, because the data is intermingled. It makes changing the data harder, because I might accidentally change the behavior while I’m at it. I tried to push some of the data into resources, but I got stuck pretty quickly. I have to admit, managed resources are pretty confusing to me. Refactoring this stays on the TODO list, and learning about resources goes there, too.
IDisposable … or not
Commenters on the last post noted that I may not be disposing Pen and Brush objects. Turns out the situation is pretty bad.
In the BorderedRectangle, it’s easy:
using (Pen pen = new Pen(this._color, this._thickness))
{
g.DrawRectangle(pen, this._rectangle);
}
But in RssItemView it’s not. There is a Pen field that isn’t disposed. It’s also replaced by the set_LineColor and set_LineWidth, which means you could potentially leak a whole lot of handles pretty quickly.
There’s not an easy way to just toss in ‘using’ at this point and make the problem go away. I am going to leave the bug in place because I’m still working on cleaning up RssView at this point. When I turn my attention to RssItemView, I’ll probably refactor it in such a way that this bug will be trivial to fix.
One commenter also suggested renaming Location to Bounds, which I’ve done. Seems reasonable.
In a discussion with Cyrus, he gave me a hard time for creating types with an underscore suffix. He’s completely justified, but I haven’t found a solution I like better. If we come across something better, I’ll apply it.