Scott Hanselman

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. ;)

About Scott

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

About   Newsletter

Hosting By