C# Form modyfing in static methods

Question

I ran into some problems with my C# program. I want to create stopwatch (countdown from certain value), that is started, when a certain key is pressed. To handle keys i use low-level keyboard hook. But this class has static methods, so if I want to call a method from different class, that is not static, i have to create a new instance. With the countdown, I want to change the Text property of the TextBox element every tick (second). The problem is, how to change the properties of TextBox every tick (in the Countdown class), when I have to create a new instance of Countdown class in the static method, thus the TextBox will no longer response to the previous TextBox. My code works perfectly fine, the keys are recognized, the timer is counting down and showing the value of seconds in separate MessageBox'es (for debug), but it won't change the text in the form.

If it would help you to understand what I wrote above, I can give you my code. Just say so in the comment.

Thanks for help in advance.

The code:

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


namespace stopwatch2
{
public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
    }

    private void Form1_Load(object sender, EventArgs e)
    {
        InterceptKeys.InterceptInit();
    }

    private void Form1_Closing(object sender, CancelEventArgs e)
    {
        InterceptKeys.Unhook();
    }

    public void changeText(string text)
    {

        MessageBox.Show(text); //for debug
        textBox1.Text = text;
    }


    class InterceptKeys
    {

        private const int WH_KEYBOARD_LL = 13;
        private const int WM_KEYDOWN = 0x0100;
        private static LowLevelKeyboardProc _proc = HookCallback;
        private static IntPtr _hookID = IntPtr.Zero;

        public static void InterceptInit()
        {
            _hookID = SetHook(_proc);
        }

        public static void Unhook()
        {
            UnhookWindowsHookEx(_hookID);
        }

        private static IntPtr SetHook(LowLevelKeyboardProc proc)
        {
            using (Process curProcess = Process.GetCurrentProcess())
            using (ProcessModule curModule = curProcess.MainModule)
            {
                return SetWindowsHookEx(WH_KEYBOARD_LL, proc,
                    GetModuleHandle(curModule.ModuleName), 0);
            }
        }

        private delegate IntPtr LowLevelKeyboardProc(
            int nCode, IntPtr wParam, IntPtr lParam);

        public static IntPtr HookCallback(
            int nCode, IntPtr wParam, IntPtr lParam)
        {


            if (nCode >= 0 && wParam == (IntPtr)WM_KEYDOWN)
            {
                int vkCode = Marshal.ReadInt32(lParam);

                Countdown timer = new Countdown(); //creating new instance

                if ((Keys)vkCode == Keys.Home)
                {

                    timer.StartTimer();

                }

                if ((Keys)vkCode == Keys.End)
                {

                    timer.StopTimer();

                }

            }
            return CallNextHookEx(_hookID, nCode, wParam, lParam);
        }

        [DllImport("user32.dll", CharSet = CharSet.Auto, SetLastError = true)]
        private static extern IntPtr SetWindowsHookEx(int idHook,
            LowLevelKeyboardProc lpfn, IntPtr hMod, uint dwThreadId);

        [DllImport("user32.dll", CharSet = CharSet.Auto, SetLastError = true)]
        [return: MarshalAs(UnmanagedType.Bool)]
        private static extern bool UnhookWindowsHookEx(IntPtr hhk);

        [DllImport("user32.dll", CharSet = CharSet.Auto, SetLastError = true)]
        private static extern IntPtr CallNextHookEx(IntPtr hhk, int nCode,
            IntPtr wParam, IntPtr lParam);

        [DllImport("kernel32.dll", CharSet = CharSet.Auto, SetLastError = true)]
        private static extern IntPtr GetModuleHandle(string lpModuleName);


    }

   public partial class Countdown : Form1
    {

        public System.Windows.Forms.Timer timer1;
        public int counter = 60;

        public void StartTimer()
        {

            timer1 = new System.Windows.Forms.Timer();
            timer1.Tick += new EventHandler(timer1_Tick);
            timer1.Interval = 1000; // 1 second
            timer1.Start();
            changeText(counter.ToString());

        }

        public void timer1_Tick(object sender, EventArgs e)
        {

            counter--;
            if (counter == 0)
                counter = 60;

            changeText(counter.ToString());

        }

        public void StopTimer()
        {
            timer1.Stop();
        }

     }

  }

}

Answer 1

So, first off, you don't want Countdown to extend Form1. This is giving you the false impression that you're able to access the members of Form1, but you just can't. You're creating an entirely new form each time you create a new Countdown instance with it's own textbox and it's own...everything.

Worse still, you create a new Countdown event each time your hook handler is fired, so you're not stopping the timer on the same instance of Countdown that you have previously started.

InterceptKeys also shouldn't be an inner class of Form1, it should be it's own stand alone class in its own file.

In all honesty, I don't even think the Countdown class should exist; it's methods should belong in one of the two other classes.

Let's start by looking at InterceptKeys. You can see that 90% of the class is just creating the hooks and determining if it's an event that you "care about". We then only have a little bit of code (that 10%) of doing whatever needs to happen when something we care about happens.

There is a way we can handle this more effectively to address "separation of concerns". The InterceptKeys class only needs to handle the boilerplace code of setting up a keyboard hook and filtering out the ones we don't care about. We would like to move the last 10% of code out of this class. Events are a great way to do this. Logically there are two "events" that happen here, one is when the home key is pressed, the other is when the end key is pressed. So we'll start by creating these two events inside of InterceptKeys:

public static event Action HomePressed;
public static event Action EndPressed;

Now we can just fire these two events instead of calling the method of another class at the appropriate locations. Just replace:

Countdown timer = new Countdown(); //creating new instance
if ((Keys)vkCode == Keys.Home)
{
    timer.StartTimer();
}
if ((Keys)vkCode == Keys.End)
{
    timer.StopTimer();
}

with:

if ((Keys)vkCode == Keys.Home)
{
    if(HomePressed != null) HomePressed();
}
if ((Keys)vkCode == Keys.End)
{
    if(EndPressed != null) EndPressed();
}

So, now InterceptKeys has two events, now what? Now we go to where InterceptKeys is initialized in Form1 and handle the events. Before we do this we'll first want to grab everything that was once inside of Countdown and and put it right in Form1, just move the whole thing. With all of those methods there we can do this:

private void Form1_Load(object sender, EventArgs e)
{
    InterceptKeys.InterceptInit();
    InterceptKeys.HomePressed += ()=> StartTimer();
    InterceptKeys.EndPressed += ()=> StopTimer();
}

Now anytime one of those two keys are pressed the appropriate method is called on the existing method of this form. On top of that, we have now moved all of the code that is manipulating the display of the form to that form's definition, while ensuring that all of that nasty keyboard hook stuff is off in it's own little world. "Separation of concerns", everything is in it's own appropriate place.

On a side note, you should really make timer1 and counter private fields, not public fields. You're not using them publicly, which is good, but you don't want to ever do so in the future. You can create methods that provide limited access to those fields as needed (which is what you're currently doing).

There's just one thing left. Chances are each time Home is pressed you don't want to start a new timer. The old timer will still be around, so then you'll have two, three, or more timers. What's more likely is that you just want to restart the existing timer. This is easy enough to do.

In StartTimer you can just not create a new timer but instead manipulate the existing one. Remove everything from it's body except:

timer1.Start();
changeText(counter.ToString());

Then just create and configure the Timer when the form is first loaded:

private void Form1_Load(object sender, EventArgs e)
{
    timer1 = new System.Windows.Forms.Timer();
    timer1.Tick += new EventHandler(timer1_Tick);
    timer1.Interval = 1000; // 1 second

    InterceptKeys.InterceptInit();
    InterceptKeys.HomePressed += ()=> StartTimer();
    InterceptKeys.EndPressed += ()=> StopTimer();
}