Winforms Dice Control

Question

This is my second Custom Control I have made, a dice. The code below has worked 100% in all my tests, program contains 1 dice control and seven buttons, 6 for setting a number directly, and one for randomizing it. Feedback and improvement suggestions are greatly appreciated.

using System;
using System.Drawing;
using System.Windows.Forms;

namespace DiceControl
{
    class Dice : Control
    {
        public int RollNumber { get { return rollNumber; } set { rollNumber = value; Invalidate(); } }
        private int rollNumber;
        private Point[][] points = new Point[6][];
        public Dice(int rollNumber = 0) : base()
        {
            this.rollNumber = rollNumber;
        }

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

            Graphics g = e.Graphics;
            Rectangle rc = ClientRectangle;
            Pen border = new Pen(Brushes.Black, 3.0f);
            Point[] RollOne = { new Point(((rc.Width / 2) - rc.Width / 8), (rc.Height / 2) - rc.Height / 8) };
            Point[] RollTwo = { new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8) };
            Point[] RollThree = { new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Width / 2) - rc.Width / 8, (rc.Height / 2) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8) };
            Point[] RollFour = { new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8) };
            Point[] RollFive = { new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point(((rc.Width / 2) - rc.Width / 8), (rc.Height / 2) - rc.Height / 8), new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8) };
            Point[] RollSix = { new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Width / 2) - rc.Width / 8), new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Width / 2) - rc.Width / 8), new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8) };
            points[0] = RollOne;
            points[1] = RollTwo;
            points[2] = RollThree;
            points[3] = RollFour;
            points[4] = RollFive;
            points[5] = RollSix;
            Point[] edges = {new Point(rc.Left,rc.Top),new Point(rc.Right-1,rc.Top),new     Point(rc.Right-1,rc.Bottom-1),new Point(rc.Left,rc.Bottom-1)};

            g.FillRectangle(Brushes.White, rc);

            if (rollNumber >= 1 && rollNumber <= 6)
            {
                foreach (Point pt in points[rollNumber-1])
                {
                    g.FillEllipse(Brushes.Black, pt.X, pt.Y, rc.Width / 4, rc.Height / 4);
                }
            }
            else
            {
                StringFormat sf = new StringFormat();
                sf.Alignment = StringAlignment.Center;
                sf.LineAlignment = StringAlignment.Center;
                g.DrawString("?", new Font("Arial",(float)rc.Height*0.5f,FontStyle.Bold,GraphicsUnit.Pixel), Brushes.Black, rc, sf);
            }

            g.DrawPolygon(border, edges);
        }
    }

    class MainWindow : Form
    {
        Dice d;
        Button one, two, three, four, five, six, roll;
        public MainWindow()
        {
            this.Text = "Dice Control";
            this.Height = 175;
            this.Width = 240;

            d = new Dice();
            d.Height = 120;
            d.Width = 120;
            d.Top = 10;
            d.Left = 10;
            this.Controls.Add(d);

            one = new Button();
            one.Left = 155;
            one.Top = 17;
            one.Text = "1";
            one.Click += (s, e) => d.RollNumber = 1;
            one.Width = 20;
            this.Controls.Add(one);

            two = new Button();
            two.Top = one.Top;
            two.Left = one.Right + 5;
            two.Text = "2";
            two.Click += (s, e) => d.RollNumber = 2;
            two.Width = 20;
            this.Controls.Add(two);

            three = new Button();
            three.Top = one.Bottom + 5;
            three.Left = one.Left;
            three.Text = "3";
            three.Click += (s, e) => d.RollNumber = 3;
            three.Width = 20;
            this.Controls.Add(three);

            four = new Button();
            four.Top = one.Bottom + 5;
            four.Left = three.Right + 5;
            four.Text = "4";
            four.Click += (s, e) => d.RollNumber = 4;
            four.Width = 20;
            this.Controls.Add(four);

            five = new Button();
            five.Top = three.Bottom + 5;
            five.Left = three.Left;
            five.Text = "5";
            five.Click += (s, e) => d.RollNumber = 5;
            five.Width = 20;
            this.Controls.Add(five);

            six = new Button();
            six.Top = four.Bottom + 5;
            six.Left = five.Right + 5;
            six.Text = "6";
            six.Click += (s, e) => d.RollNumber = 6;
            six.Width = 20;
            this.Controls.Add(six);

            roll = new Button();
            roll.Text = "Roll";
            roll.Left = 140;
            roll.Top = six.Bottom + 5;
            roll.Click += roll_Click;
            this.Controls.Add(roll);
        }

        void roll_Click(object sender, EventArgs e)
        {
            Random rand = new Random();
            d.RollNumber = rand.Next(1, 7);
        }
    }
    class Start
    {
        public static void Main()
        {
            MainWindow main = new MainWindow();
            Application.EnableVisualStyles();
            Application.Run(main);
        }
    }
}

Answer 1

There's an unofficial principle I try to abide by. I call it the "Single Page Principle", but in this case, we'll call it the "Single Screen Principle". What that means, is that any one method should fit nicely onto one screen without scrolling. Scrolling breaks Mr. Maintainer's concentration and focus.

public int RollNumber { get { return rollNumber; } set { rollNumber = value; Invalidate(); } }

This needs some breathing room and may or may not break the SSP depending on screen size, but all in all isn't too bad. Although, it would read better like this.

public int RollNumber 
{ 
    get { return rollNumber; } 
    set 
    { 
        rollNumber = value; 
        Invalidate();
    } 
}

Then there's this.......

        Point[] RollOne = { new Point(((rc.Width / 2) - rc.Width / 8), (rc.Height / 2) - rc.Height / 8) };
        Point[] RollTwo = { new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8) };
        Point[] RollThree = { new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Width / 2) - rc.Width / 8, (rc.Height / 2) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8) };
        Point[] RollFour = { new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8) };
        Point[] RollFive = { new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point(((rc.Width / 2) - rc.Width / 8), (rc.Height / 2) - rc.Height / 8), new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8) };
        Point[] RollSix = { new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Top + rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Width / 2) - rc.Width / 8), new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Width / 2) - rc.Width / 8), new Point((rc.Left + rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8), new Point((rc.Right - rc.Width / 4) - rc.Width / 8, (rc.Bottom - rc.Height / 4) - rc.Width / 8) };

Which my tiny brain can't even begin to wrap around. I do see an awful lot of duplication here though. With the exception of RollOne (which should be rollOne by the way), the first coordinate in each of them is the same. Precalculate it and store it in a variable.

int x = ((rc.Left + rc.Width / 4) - rc.Width / 8)
int y = ((rc.Top + rc.Height / 4) - rc.Width / 8)

Then take the next step and create a variable to hold that point.

point bottomLeft = new Point(x,y)

And then add that point to you point array

Point[] RollTwo = {bottomleft, //etc}

Continue this process until the code is readable and fits on the screen.

---

This code also gets repeated a lot.

        one = new Button();
        one.Left = 155;
        one.Top = 17;
        one.Text = "1";
        one.Click += (s, e) => d.RollNumber = 1;
        one.Width = 20;
        this.Controls.Add(one);

Create a method that modifies the button by passing parameters for the different values.

void AddDieButton(button die, int left, int top, int width, string text, event eventHandler)
{
    die.Left = left;
    die.Top = top;
    die.Width = width;
    die.Text = text;
    die.Click += eventHandler;
    this.Controls.Add(die);
}

Disclaimer: I ran none of this through the compiler.

---

The more I look at this the more I think you need a SixSidedDie class that is responsible for determining where the dots and edges are drawn. It's constructor should take an int from 1 to 6 as an argument.