Поделиться через

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.

Comments

  • Anonymous
    August 21, 2004
    Cyrus is right... How about ConfigurationParameter? or ConfigurationState? or ConfigurationFields? or ConfigurationEssence?

    Ah, last one reminds me of this:

    http://www.codeproject.com/csharp/EssencePattern.asp

  • Anonymous
    August 21, 2004
    The comment has been removed

  • Anonymous
    August 21, 2004
    The comment has been removed

  • Anonymous
    August 22, 2004
    Andy: It's not legal in my case because the class and member are defined in the same scope.

    These days I find myself creating lots of 1-off classes, so it makes sense to have the class defined in the scope that it'll be used it.

  • Anonymous
    August 24, 2004
    The comment has been removed

  • Anonymous
    June 01, 2008
    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 differenc

  • Anonymous
    June 05, 2008
    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 differenc

  • Anonymous
    June 16, 2009
    PingBack from http://topalternativedating.info/story.php?id=1345