Winforms Circular Checkbox

Question

So here is my next creation, a circular checkbox. This is my third custom control, so I'm still learning, but have the basics just about down. Tips and suggestions are most appreciated:

class CircularCheckbox : Control
{
    private Color borderColor;
    public Color BorderColor
    {
        get { return borderColor; }
        set { borderColor = value; }
    }

    private Color background;
    public Color Background
    {
        get { return background; }
        set { background = value; }
    }

    private Color tickColor;
    public Color TickColor
    {
        get { return tickColor; }
        set { tickColor = value; }
    }

    private bool isChecked;
    public bool IsChecked
    {
        get { return isChecked; }
        set { isChecked = value; }
    }

    public CircularCheckbox(bool IsChecked)
    {
        DoubleBuffered = true;
        SetStyle(ControlStyles.SupportsTransparentBackColor, true);
        this.BackColor = Color.Transparent;

        this.IsChecked = IsChecked;
        Background = Color.White;
        BorderColor = Color.Black;
        TickColor = Color.Green;
    }

    public CircularCheckbox()
        : this(false)
    { }

    protected override void OnPaint(PaintEventArgs e)
    {
        base.OnPaint(e);

        e.Graphics.SmoothingMode = System.Drawing.Drawing2D.SmoothingMode.AntiAlias;
        Rectangle rc = this.ClientRectangle;
        Graphics g = e.Graphics;
        StringFormat sf = new StringFormat();
        Font f = new Font("Arial", (float)rc.Height * 0.5f, FontStyle.Bold, GraphicsUnit.Pixel);

        sf.Alignment = StringAlignment.Center;
        sf.LineAlignment = StringAlignment.Center;

        g.FillEllipse(new SolidBrush(Background), rc.Left + 1.5f, rc.Top + 1.5f, rc.Width - 4.0f, rc.Height - 4.0f);
        g.DrawEllipse(new Pen(new SolidBrush(BorderColor), 3.0f), rc.Left + 1.5f, rc.Top + 1.5f, rc.Width - 4.0f, rc.Height - 4.0f);

        if (IsChecked)
            g.DrawString("\u2713", f, new SolidBrush(tickColor) , rc, sf);

    }

    protected override void OnClick(EventArgs e)
    {
        base.OnClick(e);

        IsChecked = IsChecked ? false : true;

        Invalidate();
    }
}

Answer 1

Other people have mentioned that you should use auto-properties, but they're missing another point about custom controls. When someone changes a property of your custom control which should result in a change in how your control is drawn, you need to Invalidate your control.

private bool isChecked;
public bool IsChecked
{
    get { return isChecked; }
    set
    {
        if (value == isChecked) return;
        isChecked = value;
        this.Invalidate();
    }
}

The if check ensure we're not needlessly invalidating the control when there won't be an actual change in the painting logic. Again, this should be done for any property whose value being changed means the drawing needs to be done - which in this case is all of your properties.

You should also be properly disposing of the drawing objects you're creating. The best way to do this is to create the objects in a using block. Note that you can pass a color directly to the Pen constructor instead of passing a Brush object (which would have to be disposed of properly if you went that route).

using (var backgroundBrush = new SolidBrush(Background))
using (var borderPen = new Pen(BorderColor, 3.0f))
{
    g.FillEllipse(backgroundBrush, rc.Left + 1.5f, rc.Top + 1.5f, rc.Width - 4.0f, rc.Height - 4.0f);
    g.DrawEllipse(borderPen, rc.Left + 1.5f, rc.Top + 1.5f, rc.Width - 4.0f, rc.Height - 4.0f);
}

Your Font and StringFormat instances should also be getting disposed of properly.

Another thing to keep in mind is that it is possible for other code to add a Paint event handler which will be run after you perform your painting. You're changing the SmoothingMode of your graphics object and not restoring it before you complete your painting. I'd recommend restoring the Graphics back to its original state before you complete.

// At the beginning of your drawing
var originalGraphicsState = e.Graphics.Save();
e.Graphics.SmoothingMode = SmoothingMode.AntiAlias;

/// .. draw some stuff ..

// At the end of your drawing
e.Graphics.Restore(originalGraphicsState);

Answer 2

• you should use auto properties where you don't need any validation. So all of your properties should use them like instead of

  private bool isChecked;
  public bool IsChecked
  {
      get { return isChecked; }
      set { isChecked = value; }
  }
  

  you would use

  public bool IsChecked { get; set; }
  

• this example of really unnecessary tenary expression

  IsChecked = IsChecked ? false : true;
  

  should be replaced by

  IsChecked = !IsChecked;  
  

• using braces {} for single if statement also would make your code less errorprone

• you should extract your magic numbers into some meaningful aka good named constants ( 3.0f , 1.5f etc)
• you should extract "\u2713" to some meaningful constant
• you should extract new SolidBrush(tickColor) to a field, as the brush itself won't change.

Answer 3

In general your code is good, like the fact that you implemented constructor overloading, but there's always room for improvements:

• Backfield-properties vs. Auto-implemented properties:

  You are not doing anything internally with your backing fields, like validation of value for example. So there's no use in implementing properties this way. Use auto-implemented properties instead. Following property:

  private Color borderColor;
  public Color BorderColor
  {
      get { return borderColor; }
      set { borderColor = value; }
  }
  

  would become:

  public Color BorderColor { get; set; }
  

  This also goes for the other properties.

• Naming:

  The names of your private fields for the properties have correct names, try to continue doing this for all names in your code. Don't use variable names like rc, g and sf, they don't mean anything. Use rectangle, graphics and stringFormat/format instead.

  In the constructor with the parameter, the name is capitalized. Parameter names use camelCase and not PascalCase, Make it isChecked. (More reading)

• Nitpicking:

  Although following line is not incorrect, it can be simplified.

  IsChecked = IsChecked ? false : true;
  

  would become:

  IsChecked = !IsChecked;