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.