Scott Hanselman

Learning WPF with BabySmash - Pushing things up a level with another set of eyes

June 13, '08 Comments [6] Posted in BabySmash | Learning .NET | Windows Client | WPF
Sponsored By

NOTE: If you haven't read the first post in this series, I would encourage you do to that first, or check out the BabySmash category. Also check out http://windowsclient.net/ for more developer info on WPF.

BACKGROUND: This is one of a series of posts on learning WPF. I wrote an application for my 2 year old using WPF, but as I'm a Win32-minded programmer, my working app is full of Win32-isms. It's not a good example of a WPF application even though it uses the technology. I'm calling on community (that's you, Dear Reader) to blog about your solutions to different (horrible) selections of my code. You can get the code http://www.codeplex.com/babysmash. Post your solutions on your blog, in the comments, or in the Issue Tracker and we'll all learn WPF together. Pick a single line, a section, or subsystem or the whole app!

BabySmash has a little mini-heirarchy of "Figures" that represent the Shapes on the screen, along with a little additional information like the name of the Figure ("square", "circle") as well as an English word for the color. I intend to internationalize it at some point.

Figure has been pretty simple so far and doesn't really do anything:

public abstract class Figure
{
private UIElement shape;
private readonly string name;
private readonly string color;

protected Figure(Brush fill, string name)
{
this.color = Utils.BrushToString(fill);
this.name = name;
}

public UIElement Shape
{
get { return shape; }
protected set { shape = value; }
}

public string Name { get { return name; } }
public string Color { get { return color; } }
}

Derived Shapes are mostly declarative, and very repetitive:

public class SquareFigure : Figure
{
public SquareFigure(Brush fill)
: base(fill, "square")
{
Shape = new Rectangle()
{
Fill = fill,
Height = 380,
Width = 380,
StrokeThickness = 5,
Stroke = Brushes.Black,
};
}
}

public class StarFigure : Figure
{
public StarFigure(Brush fill)
: base(fill, "star")
{
Shape = new Star()
{
NumberOfPoints = 5,
Height = 400,
Width = 400,
Fill = fill,
StrokeThickness = 5,
Stroke = Brushes.Black,
};
}
}

Last post, we removed a bunch of the Style related stuff and put it into XAML markup as a shared Style application resource we could use all over. This makes things DRYer (Don't Repeat Yourself) but still something doesn't smell right.

I still have to grab the Style and pass the Fill along into the Shape property, and I do it for EVERY Shape. Meh. It's the same four lines over and over again.

public class SquareFigure : Figure
{
public SquareFigure(Brush fill)
: base(fill, "square")
{
Shape s = new Rectangle();
s.Style = Application.Current.Resources["square"] as Style;
s.Fill = fill;
Shape = s;
}
}

Ripe for refactoring I say. Note the call to base() in the Constructor. You'd be surprised how many people either don't know about base() or who knew and forgot. ;)

Oddly, I was already passing Fill into the constructor, and then passing it up the hierarchy to the abstract base class Figure without thinking. This is very common when you slap code together then finally take a breath and LOOK at it. Ever more common when you have someone who isn't "into" the code just look over your shoulder and say "um, what's THAT?" You really can't overestimate how useful another set of eyes are. I did a quick SharedView with Jason this afternoon and his fresh eyes sped up my refactoring considerably.

public abstract class Figure
{
private readonly string name;
private readonly string color;

protected Figure(Brush fill, string name, Shape s)
{
this.color = Utils.BrushToString(fill);
this.name = name;
this.Shape = s;
s.Fill = fill;
s.Style = Application.Current.Resources[Name] as Style;
}

public UIElement Shape { get; protected set; }

public string Name { get { return name; } }
public string Color { get { return color; } }
}

Here we moved everything that was repeated (shared) up into the base class. The Shape property used the C# 3.0 property syntax that let us remove a field (it's generated now). Notice the use of the protected keyword on the setter.

The only thing that "smells" bad about this for me is the call to Application.Current.Resources. This is the moral equivalent to HttpContext.Current as it's saying "reach out into the heavens that you ought not know about and pull a rabbit out of a hat" or, in this case, a resource out of Application.Resources. However, as far as I can tell, this is a minor sin, and a common one, unless I want to start passing all these bits of information around my hierarchy while trying to obey the Law of Demeter, which I'm arguably already breaking.

Regardless, suddenly my library of Figures becomes much simpler. Of course, adding new ones will be even simpler now. For example:

public class RectangleFigure : Figure
{
public RectangleFigure(Brush fill)
: base(fill, "rectangle", new Rectangle()){}
}

public class CircleFigure : Figure
{
public CircleFigure(Brush fill)
: base(fill, "circle", new Ellipse()){}
}

public class TriangleFigure : Figure
{
public TriangleFigure(Brush fill)
: base(fill, "triangle", new Polygon()){}
}

public class StarFigure : Figure
{
public StarFigure(Brush fill)
: base(fill, "star", new Star()){}
}

In the next post I'll refactor the factory that makes these figures into something that allows me to sleep at night. ;)

Technorati Tags: ,,

About Scott

Scott Hanselman is a former professor, former Chief Architect in finance, now speaker, consultant, father, diabetic, and Microsoft employee. I am a failed stand-up comic, a cornrower, and a book author.

facebook twitter subscribe
About   Newsletter
Sponsored By
Hosting By
Dedicated Windows Server Hosting by ORCS Web
Friday, June 13, 2008 5:35:58 AM UTC
I didn't understand "The Shape property used the C# 3.0 property syntax that let us remove a field (it's generated now)", perhaps because I haven't been following the story so far in enough detail. Would any kind soul like to explain it to me?
Friday, June 13, 2008 6:09:51 AM UTC
Why do you keep saying "Don't repeat yourself"? Isn't once enough? ;-)
Palle Due Larsen
Friday, June 13, 2008 6:10:17 AM UTC
Max - Notice in the first code listing that there was a Shape property and a backing field:

public UIElement Shape
{
get { return shape; }
protected set { shape = value; }
}

But in the last listing, it's cleaner using the new syntax:

public UIElement Shape { get; protected set; }
Friday, June 13, 2008 6:10:57 AM UTC
Max, note how originally there was a private field "shape" that was later refactored out in favor of an auto-property.
Friday, June 13, 2008 6:11:22 AM UTC
Palle - I only said it once in this post. ;) I am just trying to make each post stand on its own.
Friday, June 13, 2008 2:17:51 PM UTC
Ah ha. So the compiler will auto generate a backing field for Shape, something like:

[CompilerGenerated]
private UIElement <Shape>k__BackingField; // angled brackets here are not a generic but actually part of the identifier

Thanks folks.
Comments are closed.

Disclaimer: The opinions expressed herein are my own personal opinions and do not represent my employer's view in any way.