12
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Would be nice if you added a screenshot of your creation + how it looks in all of its states, i.e. explain the code without the need to read code. \$\endgroup\$ – Neolisk Dec 17 '14 at 19:59
9
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ +1 good points especially using. For the properties I agree but not with the IsChecked because it is handled by override void OnClick(). But maybe it would be better to put the call to Invalidate() inside each property setter \$\endgroup\$ – Heslacher Dec 17 '14 at 16:23
  • \$\begingroup\$ I wrote this comment without looking at the code ;-) corrected comment. \$\endgroup\$ – Heslacher Dec 17 '14 at 16:29
  • \$\begingroup\$ My point still stands though, if some outside code calls myCustomCheckBox.IsChecked = true it will set the property but not repaint the graphics. If you want to get rid of the Invalidate in the IsChecked public setter you need to make the property setter private - or maybe protected with a good comment explaining that the caller must Invalidate the control after setting the value. \$\endgroup\$ – Anthony Dec 17 '14 at 16:34
  • \$\begingroup\$ If you see this from the point as a public avaiable assembly, then you are right, but then the class should be public, shouldn' it. \$\endgroup\$ – Heslacher Dec 17 '14 at 16:43
  • \$\begingroup\$ Even if it is internal (which this class currently is defaulting to), it means it can be changed by other code within your own assembly. When building a Form within the same assembly, you can place a CircularCheckbox in the form and then set the value from within your custom Form. Without invalidating in the property setter, all of the sudden your Form needs to know to call Invalidate after setting the property. It is the responsibility of the CircularCheckbox to know when it needs to redraw itself, which is the moment the IsChecked property is changed. \$\endgroup\$ – Anthony Dec 17 '14 at 16:48
12
\$\begingroup\$
  • 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.
\$\endgroup\$
9
\$\begingroup\$

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;
    
\$\endgroup\$
  • 2
    \$\begingroup\$ You wear a nice hat ;-) \$\endgroup\$ – Heslacher Dec 17 '14 at 10:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.