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 (8)

  1. Jason Yip says:

    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

  2. Andy says:

    "I have the underscore because otherwise there would be a conflict between the name of the property and the name of the type."

    Are you sure about this? I haven’t done much of anything substantial in C# 2.0, but in 1.1, I’ve done this, and it’s compiled without any problems:

    using System;

    using System.Reflection;

    public namespace Foo

    {

    public class Bar

    {

    public readonly Assembly Assembly;

    public Bar(Assembly assembly)

    {

    Assembly = assembly;

    }

    }

    }

    Of course, it is somewhat confusing that the only difference between the constructor parameter and the member is the case of the first letter, but since I only use the parameter once, I think that’s ok.

    I’ve also had people comment that it’s confusing for them to have both a member and a Type with the same name. Mostly this is coming from people who haven’t written much managed code, and are still doing unmanaged Win32 full time.

  3. Andy’s right – the name of the property and the type of the property

    class Bar

    {

    private Foo m_foo;

    public Foo Foo

    {

    get { return m_foo; }

    set { m_foo = value; }

    }

    }

    class Foo { }

    Pro: when your class has a 1:1 relationship with an instance of another type, re-using the type name as the property name aids discoverability and avoids the extra keystrokes of forcing people to modify the property name to be _Foo or Prop_Foo or whatever. If there’s no additional semantic to attach to the 1:1 relationship, just the "here’s the instance of the associated type", it works nicely.

    Con: I *hate* this being allowed because then you’re going through code and seeing (for property Foo of type Foo) code like Foo.SomeMethod(p1, p2); and your brain immediately says "ah, static method call over on Foo" and you’re in that mental mode. If you’re lucky you notice your IDE used a font to clue you into that the Foo there is actually a property reference. It’s not that it’s invalid code, it’s just that it can distort your internal mental code view/perception of things. Later on, you’re checking out SomeMethod, say, and notice it’s actually not static and then you waste mental cycles resolving the conflict of "hey, how was Bar calling this instance method in a static way? is there a compiler bug? am i on crack?"

    The response would seem to be "you’re in Bar, why refer to the property name instead of the backing store m_foo when calling a method?" which is valid, but ignores the (mis?)use of properties for lazy initialization, ala:

    public Foo Foo

    {

    get { if (m_foo == null) m_foo = new Foo(); return m_foo; }

    set { m_foo = value; }

    }

    An easy pattern to implement, but when done so, even the class itself will need (well, want) to convert from m_foo.SomeMethod to Foo.SomeMethod so the class gets the same initialization benefits as others.

    Then the fallback argument (assuming the "just check the font Intellisense is showing you" isn’t working) becomes "you can always disambiguate with this.Foo.SomeMethod() which is obviously the property" which is very true, but the inconsistency is then the negative (assuming for properties of type Foo called MyFoo the class would just MyFoo.SomeMethod())

    Anyway, enough babbling. In the end, I fall on the "like that it’s allowed" case, because the case of "1:1 relationship, no additional semantic to associate" is going to be N cases, and then the lazy-init-ish kinds of patterns will be some percentage P of those cases, and with P*N being the lower number of the two, inconveniencing the lower number seems like the right decision to me.

  4. jaybaz [MS] says:

    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.

  5. Matthew W. Jackson says:

    I know it seems redundant, but a possible solution would be to append the containing class’s name to the name of the nested 1-off class.

    So you could have class Foo with a property Foo.Data of type Foo.FooData.

    That’s what I would name the class if it existed outside of the nested type, but I’ve started changing my designs to use more nested classes (especially with the arrival of partial classes, since I can now put the nested class in its own file). Since you’ll rarely declare any external variables of this nested type outside of your class, and you’ll only be accessing it though it’s single property, its actual name won’t matter much (thanks to Intellisense).

    It’s almost like anonymous classes in a similar way to C’s anonymous structs would solve the problem, but there’s

    1) no good synatx for it

    2) terrible versioning implications and

    3) a problem documenting and reflecting such a nameless class (since it would need to have an autogenerated name, similar to C# 2’s iterators, but the name would need to be public so it couldn’t be ugly).

  6. Dating says:

    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

  7. Weddings says:

    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