Drawing shapes in a GDI window

Question

I've been tasked with writing a windows form application that offers some options to modify the drawing of shapes within another window. I have very little experience with event-driven programming and I'd like to improve my skill.

using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using GDIDrawer;

namespace ICA3
{
    public enum ShapeType
    {
        Square,
        Circle
    };

    public partial class master : Form
    {
        const int _HEIGHT = 600;
        const int _WIDTH = 800;
        const int _SIZE = 50;

        Color DEFAULT_COLOR = Color.Red;
        Color CURRENT_COLOR;
        CDrawer dr = new CDrawer(_WIDTH, _HEIGHT);
        shapeform sf = new shapeform();

        public master()
        {
            InitializeComponent();
        }

        private void master_btn_start_Click(object sender, EventArgs e)
        {
            CURRENT_COLOR = DEFAULT_COLOR;
            master_timer.Start();
        }

        private void master_btn_stop_Click(object sender, EventArgs e)
        {
            master_timer.Stop();
        }

        private void master_btn_color_Click(object sender, EventArgs e)
        {
            using (ColorDialog cd = new ColorDialog())
            {
                if (cd.ShowDialog() == DialogResult.OK)
                {
                    CURRENT_COLOR = cd.Color;
                }
            }
        }

        private void master_btn_shape_Click(object sender, EventArgs e)
        {
            sf.ShowDialog();
        }

        private void master_timer_Tick(object sender, EventArgs e)
        {
            Point pt = new Point();

            //borders
            if (sf._BORDER)
            {
                if (sf.shape == ShapeType.Circle)
                {
                    if (dr.GetLastMouseLeftClick(out pt))
                    {
                        dr.AddCenteredEllipse(pt, _SIZE, _SIZE, CURRENT_COLOR, 1, Color.White);
                    }
                }

                else if (sf.shape == ShapeType.Square)
                {
                    if (dr.GetLastMouseLeftClick(out pt))
                    {
                        dr.AddCenteredRectangle(pt, _SIZE, _SIZE, CURRENT_COLOR, 1, Color.White);
                    }
                }
            }

            //no borders
            else
            {
                if (sf.shape == ShapeType.Circle)
                {
                    if (dr.GetLastMouseLeftClick(out pt))
                    {
                        dr.AddCenteredEllipse(pt, _SIZE, _SIZE, CURRENT_COLOR);
                    }
                }

                else if (sf.shape == ShapeType.Square)
                {
                    if (dr.GetLastMouseLeftClick(out pt))
                    {
                        dr.AddCenteredRectangle(pt, _SIZE, _SIZE, CURRENT_COLOR);
                    }
                }
            }
        }
    }
}

using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace ICA3
{
    public partial class shapeform : Form
    {
        ShapeType _SHAPE;
        public bool _BORDER = false;

        public ShapeType shape
        {
            get { return _SHAPE; }

        }

        public bool BorderChecked
        {
            get { return shapeform_cb_border.Checked; }
        }

        public shapeform()
        {
            InitializeComponent();
        }

        private void shapeform_btn_ok_Click(object sender, EventArgs e)
        {
            this.DialogResult = DialogResult.OK;
        }

        private void shapeform_btn_cancel_Click(object sender, EventArgs e)
        {
            this.Close();
        }

        private void shapeform_cb_border_CheckedChanged(object sender, EventArgs e)
        {
            _BORDER = shapeform_cb_border.Checked;
        }

        private void shapeform_rb_circle_CheckedChanged(object sender, EventArgs e)
        {
            _SHAPE = ShapeType.Circle;
        }

        private void shapeform_rb_square_CheckedChanged(object sender, EventArgs e)
        {
            _SHAPE = ShapeType.Square;
        }

    }
}

I'm trying to learn the best practices in C# and Windows forms, so tips are much appreciated.

Answer 1

Code Review submission: It's very helpful for reviewers to be able to execute your code, so that they can observe it "in action" (and, in my case, test out possible revisions). Before I could build this code, I had to

• Create a project with the classes described (which was easy, of course)

• Reverse-engineer the forms with the controls hinted at by your handler functions (which was a pain)

• Google for the library you used (GDIDrawer), and include it in the project too. If this had been any harder, I wouldn't have bothered, and you wouldn't have gotten a review.

Obviously not getting my feedback specifically isn't the end of the world, but presumably feedback is what you're here for, so try to make it easy on us.

---

Capitalization: Naming your classes with PascalCase (instead of camelCase or alllowercase) doesn't make them inherently easier to read... It makes them easier to read because that is the convention recommended by Microsoft and used by the majority of C# programmers.

---

Separate files for enums: In my opinion, if multiple classes use an enum to communicate, that enum doesn't "belong" to either class, and merits its own file. There's no penalty for having a file that's only 6 lines long.

---

Autoproperties: this C# feature may not be obvious, but it can be very useful to both shorten your code and better manage your objects' state. The following code defines a property called Shape that is publicly visible but only privately modifiable:

public ShapeType Shape { get; private set; }

---

Selection Feedback: This is a feature recommendation more than a code issue, but I found it very helpful to display the current state of the program. Next to master_btn_shape I placed a label. Whenever the shape dialog closed, I set that label to indicate the selection (for example "Circle, no border" or "Square, border")

I did the same thing to indicate whether the program was currently "Stopped" or "Waiting for clicks".

Next to the color button, I placed a small panel. Whenever the active color changed, I changed the background color of the panel.

Speaking of the active color, I replaced CURRENT_COLOR with a property as follows. This saves the trouble of remembering to change the "indicator panel" manually from anywhere the current color can be changed.

private Color _currentColor;
private Color CurrentColor // Not a constant, so no SCREAMING_SNAKE_CASE
{
    get => _currentColor; // Could as easily be "get { return currentColor; }"
    set
    {
        _currentColor = value;
        master_pnl_currentColor.BackColor = value;
    }
}

---

Initialization: I was puzzled at first that my shapes were being drawn in red when I had specified green. It wasn't until after I'd written the above code that I realized the active color is reset every time the start button is pressed. I recommend placing that sort of default initialization in the constructor.

---

Respect "cancel": If I open the shape picker, change my selection, and then cancel out? My selection is changed anyway. Why even create a cancel button?

---

Deep if nesting: The code in master_timer_Tick is a bit hairy. This is largely unavoidable given the structure of the overall program (and the library being used), but one way to reduce it would be with an "early bail out" if there is no click location:

private void master_timer_Tick(object sender, EventArgs e)
{
    if (!dr.GetLastMouseLeftClick(out var lastClickLocation))
        return; // nothing to do here

    // method continues with only 2 layers of "if" nesting ...
}

---

Object orientation: This suggestion is a larger overhaul, but you might find it useful as an exercise. One of the principles of OOP is abstraction: "I have this thing that knows how to behave, so that I don't have to deal with the details". Along those lines, you could create shapes that know how to draw themselves, so that the Tick handler is simply

private void master_timer_Tick(object sender, EventArgs e)
{
    sf.CurrentShape.Draw(dr);
}

This requires you to have defined an interface:

public interface IShapeDrawer
{
    void Draw(CDrawer drawer);
}

And you'll probably want a base class to handle the common click location detection:

public class ShapeDrawer : IShapeDrawer
{
    public Draw(CDrawer drawer)
    {
        if (!drawer.GetLastMouseLeftClick(out var lastClickLocation))
            return;

        DrawAt(drawer, lastClickLocation);
    }
    protected abstract void DrawAt(CDrawer drawer, Point location);
}

With child classes BorderedCircleDrawer, NonBorderedSquareDrawer, and so on. Each of those must implement DrawAt, making the appropriate call to drawer.

In order to make this work, the shape picker form will need to know what the current ShapeDrawer is. One way to keep track of that might be in the properties:

public partial class ShapeForm
{
    public IShapeDrawer ShapeDrawer { get; private set; }

    private void shapeform_btn_ok_Click(object sender, EventArgs e)
    {
        select case (_shape)
        {
            case (ShapeType.Circle):
                ShapeDrawer = _hasBorder 
                    ? new BorderedCircleDrawer()
                    : new NonBorderedCircleDrawer();
            case (ShapeType.Sqaure)
                ShapeDrawer = _hasBorder 
                    ? new BorderedSquareDrawer()
                    : new NonBorderedSquareDrawer();
        }
    }
}

You'll notice that technically, the same if-nesting that we moved out of the Tick handler has been moved into this Click handler... It's up to you to decide where that complexity is best managed. I would argue it belongs in the ShapeForm (which I might rename to ShapePicker or ShapeDialog), because that's where the complexity of the related controls (check box and radio buttons) already lives.

---

Further improvements: It might be nice if the ShapeDialog behaved more like the ColorDialog. For example, in how it uses DialogResult. It's almost always a good idea to match the behavior of existing objects in the standard libraries, because if you ever share your code, folks who know how to use those objects will have a head start to using yours. (This ties into the "Respect 'cancel'" point above)

I don't love the IShapeDrawer interface I invented, in part because you have to pass around references to the CDrawer. You might peek into the GDIDrawer library itself, and see if you can tap into its CEllipse and CRectangle objects directly (instead of making your own ShapeDrawer objects. If you don't have to make different calls to the different AddCenteredFoo functions directly, you may be able to clean up the code even more.

If you go that route, you might even try to reproduce the behavior of drawing shapes on the screen without using GDIDrawer at all - you might find it easier than you think.

Even further along those lines, it might be neat to make the mouse behave more like a "paintbrush": Have a shape "follow" the mouse cursor around, and leave a copy behind whenever the mouse clicks.

---

For other reviewers: You may find this more complete version of the code helpful. It includes the necessary Windows Forms designer files to make a functioning GUI (although I'm sure it's nowhere near as pretty as OP's). I have also included some of the changes I suggested in my review - mostly around the instant feedback for the state of the application. I have not included the GDIDrawer project, but it is publicly available on Github.