Windows Forms Clock

Question

Extremely rough and laggy clock

I purposely made the timer interval less than a second in hopes for making things run smoothly but it even worsened the situation! I thought of making the seconds hand run on milliseconds but it still didn't do the job and there was difference between the clock and the label text.

public Form1()
     {
            InitializeComponent();
            timer1.Interval = 500;
            timer1.Start();
    }
    private void timer1_Tick(object sender, EventArgs e)
    {
        this.Invalidate();
        this.label1.Text = String.Format("{0}:{1}:{2}",clockTime.Hour,clockTime.Minute,clockTime.Second);
   
    }
    DateTime clockTime = DateTime.Now;        
    int cirDia = 200;
    public void Form1_Paint(object sender, PaintEventArgs e)
    {
        clockTime = DateTime.Now;

        Pen secHand = new Pen(Color.Green, 1);
        Pen minHand = new Pen(Color.Black, 2);
        Pen hourHand = new Pen(Color.Red, 3);

        Graphics canvas = e.Graphics;
        float centerX = this.ClientRectangle.Width / 2;
        float centerY = this.ClientRectangle.Height / 2;
        canvas.DrawEllipse(Pens.Aqua, centerX - cirDia / 2, centerY - cirDia / 2, cirDia, cirDia);

        float secX = 100 * (float)Math.Cos(Math.PI / -2 + (2 * clockTime.Second * Math.PI) / 60) + centerX;
        float secY = 100 * (float)Math.Sin(Math.PI / -2 + (2 * clockTime.Second * Math.PI) / 60) + centerY;

        float minX = 80 * (float)Math.Cos(Math.PI / -2 + (2 * clockTime.Minute * Math.PI) / 60) + centerX;
        float minY = 80 * (float)Math.Sin(Math.PI / -2 + (2 * clockTime.Minute * Math.PI) / 60) + centerY;

        float hourX = 70 * (float)Math.Cos(Math.PI / -2 + (2 * clockTime.Hour * Math.PI) / 12) + centerX;
        float hourY = 70 * (float)Math.Sin(Math.PI / -2 + (2 * clockTime.Hour * Math.PI) / 12) + centerY;

        canvas.DrawLine(secHand, centerX, centerY, secX, secY);
        canvas.DrawLine(minHand, centerX, centerY, minX, minY);
        canvas.DrawLine(hourHand, centerX, centerY, hourX, hourY);

    }

Answer 1

Timer

The clock-face and label are 'out of sync' because Invalidate() does not cause the window to draw immediately, so clockTime = DateTime.Now; is not run before the label updates: this means the label is always 1 timer-tick behind the face. You can sovle this either by updating clockTime in the timer (I'd recommend), or by replacing this.Invalidate() with this.Refresh() which does force the painting straight away (it's a kind of dodgy method, since painting has to go through the message pump, but you are already occupying that thread...).

You probably want to increase the tick frequency further, as timers are notoriously inaccurate, and updating every 500ms means that at some point there will be a 500ms or 1500ms delay between hand movements, which will not look good. I would suggest updating clockTime in timer1_Tick before doing anything else (you could even check if a re-draw is necessary, so that you don't actually perform any update is the second counter hasn't changed).

private void timer1_Tick(object sender, EventArgs e)
{
    clockTime = DateTime.Now;
    this.label1.Text = String.Format("{0}:{1}:{2}", clockTime.Hour, clockTime.Minute, clockTime.Second);
    this.Refresh();
}

Flicker

When I run your code, the clock-face flickers when it renders. The quick and easy solution to this is to enable Double Buffering on the form. This means that when you do the painting, you are painting on an 'off-screen' canvas, which is copied onto the screen when it is completed (the flicker is it being show after the control is cleared, but before the hands are drawn). You can always implement your own double-buffering, but there is no need for such complexity here.

this.DoubleBuffered = true;

Drawing Hands

You have a lot of similar looking code here, which is just begging to be put into a method.

float secX = 100 * (float)Math.Cos(Math.PI / -2 + (2 * clockTime.Second * Math.PI) / 60) + centerX;
float secY = 100 * (float)Math.Sin(Math.PI / -2 + (2 * clockTime.Second * Math.PI) / 60) + centerY;
// etc.

Each line is computing how far round the hand went, turning this into radians, offseting that angle by -pi/2, and then computing the x or y displacement accordingly, multiplied by the hand length, and offset by the center. That's a lot to repeat on 6 lines. How about a DrawHand method?

Instead of inlining all the logic, we can separate out the bit that is concerned with drawing the hand from the bit that is concerned with working out how far round the hand has gone.

private static void DrawHand(Graphics g, PointF center, Pen pen, float handLength, float handProgress)
{
    // compute angle from handProgress
    double angleRadians = (Math.PI / -2 + (2 * handProgress * Math.PI));

    // determine x and y displacements from center
    float endX = handLength * (float)Math.Cos(angleRadians) + center.X;
    float endY = handLength * (float)Math.Sin(angleRadians) + center.Y;

    // draw line with given pen
    g.DrawLine(pen, center.X, center.Y, endX, endY);
}

Form1_Paint

As Jesse C. Slicer has already commented, you should be disposing your Pen objects (but NOT the Graphics object when you receive it from an event!). The most tidy way to do this is to use using blocks, which define the scope of the object and dispose it for you even if an exception is thrown. Putting this together with DrawHand method, you might have something like this:

public void Form1_Paint(object sender, PaintEventArgs e)
{
    Graphics canvas = e.Graphics;

    float centerX = this.ClientRectangle.Width / 2;
    float centerY = this.ClientRectangle.Height / 2;
    PointF center = new PointF(centerX, centerY);

    canvas.DrawEllipse(Pens.Aqua, centerX - circleDiameter / 2, centerY - circleDiameter / 2, circleDiameter, circleDiameter);

    using (Pen secHand = new Pen(Color.Green, 1))
    using (Pen minHand = new Pen(Color.Black, 2))
    using (Pen hourHand = new Pen(Color.Red, 3))
    {
        DrawHand(canvas, center, secHand, 100f, clockTime.Second / 12f);
        DrawHand(canvas, center, minHand, 80f, clockTime.Minute / 60f);
        DrawHand(canvas, center, hourHand, 70f, clockTime.Hour / 12f);
    }
}

Note how much cleaner and clearer the bit that draws the hand is: you could can spot the bug in seconds (pun intended) because this code only says what to draw, not how to draw it.

Answer 2

You should give your controls, properties, fields and methods descriptive names instead of using the default names given by the designer - for instance:

timer1 could be WatchTimer

label1 could be NumericDisplay

etc.

---

You should split the paint event handler into sub methods handling each part of the display:

public void MainForm_Paint(object sender, PaintEventArgs e)
{
  DrawDial(e);
  DrawHands(e);
}

---

You can optimize different things:

The pens are never changed, so they are candidates for instance fields instead of being recreated each time they are used.

  public partial class MainForm : Form
  {
    Pen _secHandPen = new Pen(Color.Green, 1);
    Pen _minHandPen = new Pen(Color.Black, 2);
    Pen _hourHandPen = new Pen(Color.Red, 3);
    ...

You then have to remember to dispose of them when disposing the form (Dispose() can befound in "Form.Designer.cs"):

/// <summary>
/// Clean up any resources being used.
/// </summary>
/// <param name="disposing">true if managed resources should be disposed; otherwise, false.</param>
protected override void Dispose(bool disposing)
{
  if (disposing)
  {
    if (components != null)
      components.Dispose();

    DisposePen(ref _secHandPen);
    DisposePen(ref _minHandPen);
    DisposePen(ref _hourHandPen);
  }
  base.Dispose(disposing);
}

private void DisposePen(ref Pen pen)
{
  if (pen != null)
  {
    pen.Dispose();
    pen = null;
  }
}

The dimensions and center of the hands and the dial only change when the size of the form changes, so create the needed class fields and handle changes in the Form_SizedChanged event handler:

private void MainForm_SizeChanged(object sender, EventArgs e)
{
  SetDimensions();
  Refresh();
}

private void SetDimensions()
{
  _center = new PointF(ClientSize.Width / 2, ClientSize.Height / 2);
  _watchDiameter = (int)((ClientSize.Height - NumericDisplay.Height < ClientSize.Width ? ClientSize.Height - NumericDisplay.Height : ClientSize.Width) * 0.9);
}

Here _center and _watchDiameter are defined as class fields:

DateTime _clockTime = DateTime.Now;

PointF _center;
int _watchDiameter = 200;

And NumericDisplay (label1) is docked to the bottom of the form, so when calculating the optimal watch size its height must be considered.

---

All in all a refactor of your form including the above and some of VisualMelons suggestions could be:

  public partial class MainForm : Form
  {
    DateTime _clockTime = DateTime.Now;

    PointF _center;
    int _watchDiameter = 200;

    Pen _secHandPen = new Pen(Color.Green, 1);
    Pen _minHandPen = new Pen(Color.Black, 2);
    Pen _hourHandPen = new Pen(Color.Red, 3);

    public MainForm()
    {
      InitializeComponent();

      SetDimensions();

      WatchTimer.Enabled = true;
      WatchTimer.Interval = 500;
      WatchTimer.Start();
    }

    private void WatchTimer_Tick(object sender, EventArgs e)
    {
      _clockTime = DateTime.Now;
      NumericDisplay.Text = _clockTime.ToLongTimeString(); // String.Format("{0:00}:{1:00}:{2:00}", _clockTime.Hour, _clockTime.Minute, _clockTime.Second);
      Refresh();
    }

    void DrawDial(PaintEventArgs e)
    {
      e.Graphics.DrawEllipse(Pens.DarkBlue, _center.X - _watchDiameter / 2, _center.Y - _watchDiameter / 2, _watchDiameter, _watchDiameter);
    }

    void DrawHands(PaintEventArgs e)
    {
      DrawHand(e, _hourHandPen, (int)(_watchDiameter * 0.3), _clockTime.Hour, 12);
      DrawHand(e, _minHandPen, (int)(_watchDiameter * 0.45), _clockTime.Minute, 60);
      DrawHand(e, _secHandPen, (int)(_watchDiameter * 0.45), _clockTime.Second, 60);
    }

    void DrawHand(PaintEventArgs e, Pen pen, int offset, int timeValue, int denom)
    {
      PointF end = GetEnd(offset, timeValue, denom);
      e.Graphics.DrawLine(pen, _center, end);
    }

    PointF GetEnd(int offset, int timeValue, int denom)
    {
      double angle = Math.PI / -2 + (2 * timeValue * Math.PI) / denom;

      return new PointF(
        offset * (float)Math.Cos(angle) + _center.X,
        offset * (float)Math.Sin(angle) + _center.Y);
    }

    public void MainForm_Paint(object sender, PaintEventArgs e)
    {
      DrawDial(e);
      DrawHands(e);
    }

    private void MainForm_SizeChanged(object sender, EventArgs e)
    {
      SetDimensions();
      Refresh();
    }

    private void SetDimensions()
    {
      _center = new PointF(ClientSize.Width / 2, ClientSize.Height / 2);
      _watchDiameter = (int)((ClientSize.Height - NumericDisplay.Height < ClientSize.Width ? ClientSize.Height - NumericDisplay.Height : ClientSize.Width) * 0.9);
    }
  }

For completeness here is the designer code with the updated names and event handlers etc.:

  partial class MainForm
  {
    /// <summary>
    /// Required designer variable.
    /// </summary>
    private System.ComponentModel.IContainer components = null;

    /// <summary>
    /// Clean up any resources being used.
    /// </summary>
    /// <param name="disposing">true if managed resources should be disposed; otherwise, false.</param>
    protected override void Dispose(bool disposing)
    {
      if (disposing)
      {
        if (components != null)
          components.Dispose();

        DisposePen(ref _secHandPen);
        DisposePen(ref _minHandPen);
        DisposePen(ref _hourHandPen);
      }
      base.Dispose(disposing);
    }

    private void DisposePen(ref Pen pen)
    {
      if (pen != null)
      {
        pen.Dispose();
        pen = null;
      }
    }

    #region Windows Form Designer generated code

    /// <summary>
    /// Required method for Designer support - do not modify
    /// the contents of this method with the code editor.
    /// </summary>
    private void InitializeComponent()
    {
      this.components = new System.ComponentModel.Container();
      this.WatchTimer = new System.Windows.Forms.Timer(this.components);
      this.NumericDisplay = new System.Windows.Forms.Label();
      this.SuspendLayout();
      // 
      // WatchTimer
      // 
      this.WatchTimer.Tick += new System.EventHandler(this.WatchTimer_Tick);
      // 
      // NumericDisplay
      // 
      this.NumericDisplay.Dock = System.Windows.Forms.DockStyle.Bottom;
      this.NumericDisplay.Location = new System.Drawing.Point(0, 437);
      this.NumericDisplay.Name = "NumericDisplay";
      this.NumericDisplay.Size = new System.Drawing.Size(800, 13);
      this.NumericDisplay.TabIndex = 0;
      this.NumericDisplay.Text = "09:41:37";
      this.NumericDisplay.TextAlign = System.Drawing.ContentAlignment.MiddleCenter;
      // 
      // TheForm
      // 
      this.AutoScaleDimensions = new System.Drawing.SizeF(6F, 13F);
      this.AutoScaleMode = System.Windows.Forms.AutoScaleMode.Font;
      this.ClientSize = new System.Drawing.Size(800, 450);
      this.Controls.Add(this.NumericDisplay);
      this.DoubleBuffered = true;
      this.Name = "MainForm";
      this.Text = "Watch";
      this.SizeChanged += new System.EventHandler(this.MainForm_SizeChanged);
      this.Paint += new System.Windows.Forms.PaintEventHandler(this.MainForm_Paint);
      this.ResumeLayout(false);

    }

    #endregion

    private System.Windows.Forms.Timer WatchTimer;
    private System.Windows.Forms.Label NumericDisplay;
  }

---

There are still a couple of "magic numbers" in the code:

void DrawHands(PaintEventArgs e)
{
  DrawHand(e, _hourHandPen, (int)(_watchDiameter * 0.3), _clockTime.Hour, 12);
  DrawHand(e, _minHandPen, (int)(_watchDiameter * 0.45), _clockTime.Minute, 60);
  DrawHand(e, _secHandPen, (int)(_watchDiameter * 0.45), _clockTime.Second, 60);
}

To get rid of them you could create a WatchHand type to hold those with descriptive names.