Clicking game in windows forms

Question

I've made a simple clicking game where you spam a button in order to get coins which you can use to buy upgrades to get even more coins.

I've only 2 classes one that is responsible for the game logic - MainGame and one for the items available in the shop - Item.

MainGame

public partial class MainGame : Form
{
    private readonly Item[] purchasableItems = new Item[20];

    private long _currentMoney = 0;
    private long currentMoney
    {
        get { return _currentMoney; }
        set
        {
            _currentMoney = value;
            lbCurrentMoney.Text = $@"{currentMoney} $";
        }
    }

    private long _moneyPerClick = 1;
    private long moneyPerClick
    {
        get { return _moneyPerClick; }
        set
        {
            _moneyPerClick = value;
            bGetMoney.Text = $@"{moneyPerClick} $";
        }
    }

    private Thread countThread = null;
    private bool stop = false;

    public MainGame()
    {
        InitializeComponent();
        Initialize();
    }

    private void Initialize()
    {
        lbCurrentMoney.Text = $@"{currentMoney} $";
        bGetMoney.Text = $@"{moneyPerClick} $";
        for (int i = 0; i < purchasableItems.Length; i++)
        {
            purchasableItems[i] = new Item((long) Math.Pow(2, i + 1), (long) (Math.Pow(2, i + 2)*10), flpShop,
                OnShopItemClickableMouseDown, OnShopItemMouseUp);
        }
        UpdateItemAccessibility();
    }

    private void OnShopItemMouseUp(object s, EventArgs e, Item item)
    {
        stop = true;
        UpdateItemAccessibility();
    }

    private void OnShopItemClickableMouseDown(object s, EventArgs e, Item item)
    {
        stop = false;
        countThread = new Thread(() =>
        {
            while (!stop && currentMoney >= item.RequiredMoney)
            {
                if (lbCurrentMoney.InvokeRequired && bGetMoney.InvokeRequired)
                {
                    Invoke(new MethodInvoker(() =>
                    {
                        currentMoney -= item.RequiredMoney;
                        moneyPerClick += item.DollarsPerClick;
                    }));
                }
                Thread.Sleep(100);
            }
        });
        countThread.Start();
    }

    private void bGetMoney_Click(object sender, EventArgs e)
    {
        currentMoney += moneyPerClick;
        UpdateItemAccessibility();
    }

    private void UpdateItemAccessibility()
    {
        foreach (Item item in purchasableItems)
        {
            item.UpdateAccessibility(currentMoney);
        }
    }
}

Item

public class Item
{
    public delegate void ShopItemEvent(object sender, EventArgs e, Item item);

    public long DollarsPerClick { get; }
    public long RequiredMoney { get; }

    private Button buttonActivator;

    public Item(long dollarsPerClick, long requiredMoney, Control parent, ShopItemEvent onMouseDown, ShopItemEvent onMouseUp)
    {
        DollarsPerClick = dollarsPerClick;
        RequiredMoney = requiredMoney;
        CreateBuyButton(parent, onMouseDown, onMouseUp);
    }

    private void CreateBuyButton(Control parent, ShopItemEvent onMouseDown, ShopItemEvent onMouseUp)
    {
        buttonActivator = new Button
        {
            Text = $@"{DollarsPerClick} $ per click." + Environment.NewLine +
                   $@"Cost : {RequiredMoney}",
            Size = new Size(parent.Width - 10, 40),
            Enabled = false
        };
        buttonActivator.MouseDown += (sender, EventArgs) => onMouseDown(sender, EventArgs, this);
        buttonActivator.MouseUp += (sender, EventArgs) => onMouseUp(sender, EventArgs, this);
        parent.Controls.Add(buttonActivator);
    }

    public void UpdateAccessibility(long currentMoney)
    {
        buttonActivator.Enabled = currentMoney >= RequiredMoney;
    }
}

The code is quite short but I think that there are plenty of stuff to improve.

Answer 1

private long _currentMoney = 0;
private long currentMoney
{
    get { return _currentMoney; }
    set
    {
        _currentMoney = value;
        lbCurrentMoney.Text = $@"{currentMoney} $";
    }
}

Looks like the only reason the private properties in this class exist is to update the text of some labels. While this isn't "wrong", it feels awkward, like you're only using the property construct for a convenient side effect. Personally I would rather create a method that updates the private field and sets the label's text, I feel that would be a more appropriate tool for this scenario.

$@"{currentMoney} $"

You're using the @ symbol for every string. It just adds one more character for my brain to parse and wonder what it's escaping. Don't use it unless it actually makes a difference.

private Thread countThread = null;

There's no reason (shown in the code, at least) for this to be a member field rather than a local in the single method that actually uses it.

private bool stop = false;
...
while (!stop && currentMoney >= item.RequiredMoney) // executed in another thread

stop should be marked volatile, otherwise the read of the stop variable in the loop could be hoisted out of the loop and only read once, rather than on each iteration.

public MainGame()
{
    InitializeComponent();
    Initialize();
}

Second method could use a better name; the first one is initializing the "component", the other is initializing... what? (It would be fine if only an Initializing method was being called, but with both I think it's confusing.)

OnShopItemMouseUp(...)
OnShopItemClickableMouseDown(...)

Those two methods could use better names too. Normally I'm fine with "OnXEvent" names, but these aren't actually being subscribed to the events... the Item class shows an anonymous function is subscribed to each event, which in turn calls these functions. Therefore I think these should be named for what they do, rather than for when they're called.

private void OnShopItemMouseUp(object s, EventArgs e, Item item)
private void OnShopItemClickableMouseDown(object s, EventArgs e, Item item)

You're not actually using the s or e parameters anywhere in these functions, and they don't need to fit any existing delegate signature (apart from your own which you can change) since they're not subscribed directly to the events, so it's pointless to have them. Remove both and leave just the Item.

private void OnShopItemClickableMouseDown(object s, EventArgs e, Item item)
{
    stop = false;
    countThread = new Thread(() =>
    {
        while (!stop && currentMoney >= item.RequiredMoney)
        {
            if (lbCurrentMoney.InvokeRequired && bGetMoney.InvokeRequired)
            {
                Invoke(new MethodInvoker(() =>
                {
                    currentMoney -= item.RequiredMoney;
                    moneyPerClick += item.DollarsPerClick;
                }));
            }
            Thread.Sleep(100);
        }
    });
    countThread.Start();
}

This is called every time some button is clicked, right? So you create a thread, then another, then another... each spends nearly 100% of its life sleeping. Use Tasks instead. And don't sleep on the thread - it means you'll have potentially hundreds of threads using memory but doing absolutely nothing. Use Timers instead, have them fire a Task on the thread pool every 100 ms, and probably a single thread pool thread can service all the requests.

if (lbCurrentMoney.InvokeRequired && bGetMoney.InvokeRequired)

Never used that before but as far as I can tell, it tells you whether you can update the control on the current thread. Given that this code ALWAYS executes on a thread which cannot update the controls directly, that check is redundant. Also, if it turned out that you don't have to invoke on the UI thread, you're doing nothing - presumably you should be updating the controls from the current thread, right?

public delegate void ShopItemEvent(object sender, EventArgs e, Item item);

As stated earlier, you may as well remove the object/EventArgs parameters since you don't use them. I know people tend to like having a sender and event args on events but rarely see them used outside actual UI-generated events, so don't see the point.

public Item(long dollarsPerClick, long requiredMoney, ...

Is money in dollars? Then be consistent in naming: requiredDollars (or possibly costInDollars or dollarCost since it appears to relate to the cost?).

    buttonActivator.MouseDown += (sender, EventArgs) => onMouseDown(sender, EventArgs, this);
    buttonActivator.MouseUp += (sender, EventArgs) => onMouseUp(sender, EventArgs, this);

EventArgs should be camelCase since it's the name of a parameter.

---

Here is an example of using Timers (from System.Threading, not any other namespace where timers may exist) rather than Threads (not the best but I don't really know how everything is called and used, so a very basic example):

private List<Timer> timers = new List<Timer>();
private readonly object syncLock = new object();
...
private void OnX(Item item)
{
    Timer timer = null;
    timer = new Timer(x =>
    {
        if (!stop && currentMoney >= item.RequiredMoney)
        {
            Invoke(new MethodInvoker(() =>
            {
                currentMoney -= item.RequiredMoney;
                moneyPerClick += item.DollarsPerClick;
            }));
        }
        else
        {
            timer.Dispose();
            lock (syncLock)
                timers.Remove(timer);
        }
    });

    lock (syncLock)
        timers.Add(timer);

    timer.Change(0, 100);
}

Basically this creates a Timer, gives it a delegate to run, and starts it (Change(0 /*Start immediately*/, 100 /*recur every 100 ms*/)). The timer executes the delegate on a thread pool thread, i.e. one of the threads that are always running in the background, waiting for jobs to do. The timer is added to a list, because the timer would otherwise get garbage collected, which would stop it. Adding it to the list keeps it rooted. When the timer should no longer run the delegate, the code that the Timer started turns around and disposes the Timer, and removes it from the list so it can get GC'd. Both the add and remove are locked for the sake of thread safety.