C# Timer (TimeSpan) keeps ticking in background (if statements apply when not true)

2
votes

I am trying to make very simple timer application. I do have a "text" object that's displaying the time after its being converted to string using: .ToString("hh\:mm\:ss");

Unfortunately after clicking the button for the whole event to start again - the if statements are being executed as if the timer was just repeating itself (keeping the old tick and text values in background) so if statements actions starts overlapping each other :(.

I have windows.forms.timer called timer2 placed in application. I have also a button called Button01 and text objects called Button01text1 & Button01textleft. The background colours and timer stop events are based on the text values comparison.

OLD CODE (invalid usage of string) :

        private void Button01_click(object sender, EventArgs e)
    {

        var startTime = DateTime.Now;
        Button01.BackColor = Color.FromName("Green");
        Button01textleft.BackColor = Color.FromName("Green");

        timer2.Tick += (obj, args) =>
        {
            Button01text1.Text =
                (TimeSpan.FromMinutes(1) - (DateTime.Now - startTime))
                .ToString("hh\\:mm\\:ss");
            Button01textleft.Text =
                (TimeSpan.FromMinutes(1) - (DateTime.Now - startTime))
                .ToString("hh\\:mm\\:ss");
            if (Button01text1.Text == "00:00:30")
            {
                Button01.BackColor = Color.FromName("Orange");
                Button01textleft.BackColor = Color.FromName("Orange");
            }
            else if (Button01text1.Text == "00:00:00")
            {
                Button01.BackColor = Color.FromName("Red");
                Button01textleft.BackColor = Color.FromName("Red");
                timer2.Stop();
            }
        };

        timer2.Enabled = true;

    }

New code (same issue, but updated thanks to @Dleh):

        public void Button01_Click(object sender, EventArgs e)
    {


        Timer timer1 = new System.Windows.Forms.Timer(); 

        var startTime = DateTime.Now;
        Button01text1.BackColor = Color.FromName("Green");
        Button01textleft.BackColor = Color.FromName("Green");

        timer1.Tick += (obj, args) =>
        {
            var now = DateTime.Now;
            var timeDifference = (TimeSpan.FromSeconds(30) - (now - startTime));
            var stringValue = timeDifference.ToString("hh\\:mm\\:ss");
            Button01text1 = stringValue;
            Button01textleft.Text = stringValue;
            if (timeDifference <= TimeSpan.FromSeconds(15))
            {
                Button01text1.BackColor = Color.FromName("Orange");
                Button01textleft.BackColor = Color.FromName("Orange");
            }
            else if (timeDifference <=TimeSpan.FromSeconds(0))
            {
                Button01text1.BackColor = Color.FromName("Red");
                Button01textleft.BackColor = Color.FromName("Red");
                timer1.Stop();
            }
        };
        timer1.Enabled = true;

    }

Now to give example what happens:

I press button once - it turns green, at 30 seconds left it turns orange at 00 it turns red and stops timer.

If I press button again in the middle of counting (at 40 seconds) it will turn to green & back to 60 seconds, but will change to orange at 50 seconds left (as if the previous tick still count down and reached 30 if I had not clicked the button again).

I am clueless, don't know why its happening - cause it's supposed to be checking the string text values - which shouldn't exist as separate instances...

Any ideas ?_?

Example Video of what's happening: Screen_recording

Maria

4
c#timer
Just a well intentioned idea: Please do not compare DateTime-format data in string format. This might haunt you later on. Use proper data-types for the data you need in your program. – Paul Weiland
Right, that sounds like a good idea - I will take a look how I can compare them in original format asap, still - I don't have any other place to "store" them - that's why I used string Text values - as those are there already being displayed on the screen. – Maria Nowinska
@MariaNowinska, when clicking on button multiple times, you subscribe to the Tick event many handlers, which have different startTime. those handlers are not synchronized and create stange effects. – ASh
How can restart it and revert timer back to 0, or remove handlers/destroy tick event? – Maria Nowinska

4 Answers

2
votes

The reason it is behaving strangely is because every time you click the button, you add another timer tick event to the timer.

This code:

timer2.Tick += (obj, args) =>

does not simply assign an event to the timer tick, it adds an event.

If you change the timer interval to 5000 and put a breakpoint in the tick event, then click the button twice, then you will see it reaches that breakpoint twice every 5 seconds.

1
votes

This should work:

private DateTime startTime;

private void timerTick(Object obj, EventArgs args) {
    Button01text1.Text =
        (TimeSpan.FromMinutes(1) - (DateTime.Now - startTime))
        .ToString("hh\\:mm\\:ss");
    Button01textleft.Text =
        (TimeSpan.FromMinutes(1) - (DateTime.Now - startTime))
        .ToString("hh\\:mm\\:ss");
    if (Button01text1.Text == "00:00:30")
    {
        Button01.BackColor = Color.FromName("Orange");
        Button01textleft.BackColor = Color.FromName("Orange");
    }
    else if (Button01text1.Text == "00:00:00")
    {
        Button01.BackColor = Color.FromName("Red");
        Button01textleft.BackColor = Color.FromName("Red");
        timer2.Stop();
    }
}

public void Button01_Click(object sender, EventArgs e)
{
    startTime = DateTime.Now;
    Button01.BackColor = Color.FromName("Green");
    Button01textleft.BackColor = Color.FromName("Green");
    timer2.Tick -= timerTick;
    timer2.Tick += timerTick;
    timer2.Enabled = true;
}

Here is a more general solution for multiple timers as promissed:

// State of specific counter
private class Counter
{
    public Timer timer;
    public DateTime startTime;
    public Button button;
    public Label text;
    public Label textLeft;
}

// List of counters
private List<Counter> counters;

private void Form1_Load(object sender, EventArgs e)
{
    // Initialize counters
    counters = new List<Counter>();
    counters.Add(new Counter { timer = timer1, button = Button01, text = Button01text, textLeft = Button01textleft });
    counters.Add(new Counter { timer = timer2, button = Button02, text = Button02text, textLeft = Button02textleft });
    counters.Add(new Counter { timer = timer3, button = Button03, text = Button03text, textLeft = Button03textleft });
    counters.Add(new Counter { timer = timer4, button = Button04, text = Button04text, textLeft = Button04textleft });
    counters.Add(new Counter { timer = timer5, button = Button05, text = Button05text, textLeft = Button05textleft });
    // Add more if you need

    // Attach handlers
    foreach (var counter in counters)
    {
        counter.timer.Tick += timerTick;
        counter.button.Click += buttonClick;
    }
}

private void timerTick(Object sender, EventArgs args)
{
    // Prepare context
    var timer = (Timer) sender;
    var counter = counters.Find(c => c.timer == timer);
    var startTime = counter.startTime;
    var button = counter.button;
    var text = counter.text;
    var textLeft = counter.textLeft;

    // Update time
    var time = TimeSpan.FromMinutes(1) - (DateTime.Now - startTime);
    text.Text = time.ToString("hh\\:mm\\:ss");
    textLeft.Text = time.ToString("hh\\:mm\\:ss");
    if (time.TotalSeconds < 1)
    {
        button.BackColor = Color.FromName("Red");
        textLeft.BackColor = Color.FromName("Red");
        timer.Stop();
    }
    else if (time.TotalSeconds < 31)
    {
        button.BackColor = Color.FromName("Orange");
        textLeft.BackColor = Color.FromName("Orange");
    }
}

public void buttonClick(object sender, EventArgs e)
{
    // Prepare context
    var button = (Button)sender;
    var counter = counters.Find(c => c.button == button);
    var timer = counter.timer;
    var textLeft = counter.textLeft;

    // Start counter
    counter.startTime = DateTime.Now;
    button.BackColor = Color.FromName("Green");
    textLeft.BackColor = Color.FromName("Green");
    timer.Enabled = true;
}
1
votes

You shouldn't be checking the string value of the time. Sometimes ticks will happen and "00:00:30" will not hit perfectly. In addition, every time you do a DateTime.Now you will get a slightly different result. You should store get it once and use that for any calculations. You should do comparisons with the time objects not the string value.

You can reinitialize your timer2 to have it stop / restart.

private void Button01_click(object sender, EventArgs e)
{
    //reinintialize your timer to stop the old one.
    timer2 = new System.Windows.Forms.Timer();

    var startTime = DateTime.Now;
    Button01.BackColor = Color.FromName("Green");
    Button01textleft.BackColor = Color.FromName("Green");

    timer2.Tick += (obj, args) =>
    {
        var now = DateTime.Now;
        var timeDifference = (TimeSpan.FromMinutes(1) - (now - startTime));
        var stringValue = timeDifference.ToString("hh\\:mm\\:ss");
        Button01text1.Text = stringValue;
        Button01textleft.Text = stringValue;

        if (timeDifference <= TimeSpan.FromSeconds(30))
        {
            Button01.BackColor = Color.FromName("Orange");
            Button01textleft.BackColor = Color.FromName("Orange");
        }
        else if (timeDifference <= TimeSpan.FromSeconds(0))
        {
            Button01.BackColor = Color.FromName("Red");
            Button01textleft.BackColor = Color.FromName("Red");
            timer2.Stop();
        }
    };
    timer2.Enabled = true;
}
0
votes

What is happening here is you keep adding more event listeners every time you click the button and they are interacting in strange ways. Because each event can have multiple listeners, every one of them fires whenever the event happens (in this case Tick). So the first time you click your button you add lambda A as a listener. It happily goes along doing what you want. The next time you click your button, you add a second listener (B) and things get weird.

This is what happens when you start again. Handler A executes and changes the time to 40 seconds, does its if checks and does nothing (the text is already green from your button click and this doesn't change it). Then handler B executes and immediately changes the time to 60 seconds (also leaves the color alone). This happens on every tick so both handlers run and change the time back and forth. After some time A will execute and change the time to 30 seconds. Then it will change the color to orange. Then B will run, changing the time back to 50, but not changing the color to green (it just leaves the color as whatever it used to be). This continues, causing the colors to change in strange ways.

To fix this what you need to do is remove the previous handlers when you click your button.