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.