5
\$\begingroup\$

I've recently started to delve into the world of windows phone 8 dev, and as a learning experience I've decided to make a stopwatch application. Now, given I've been a web guy most of my career, I've never come across UI thread locking issues, so the solution I've come up with to ensure my stopwatch class is updating the UI thread 'correctly' is (in my opinion at least) in need of a review.

So my overall questions with the below code are;

  1. Am I doing it "right"? I realise that can be kind of broad; I just want to make sure I'm not committing any cardinal sins of UI development.

  2. Are there simpler approaches to what I've done?

  3. Are there any improvements to the code that someone would suggest? I'm not sure using thread.sleep is a good idea, but when I take it out, it ends up locking the UI thread again. Hence why I'm asking for input.

My StopWatch Class:

public class StopwatchClass : IDisposable
{
    DispatcherTimer _timer;

    public int Interval { get; set; }

    public int Seconds { get; set; }
    public int Minutes { get; set; }
    public int Hours { get; set; }

    public bool IsRunning { get { return _timer.IsEnabled; } }
    public TimeSpan Elapsed { get; set; }
    public List<TimeSpan> Splits { get; set; }

    public StopwatchClass(int interval)
    {
        this.Splits = new List<TimeSpan>();

        _timer = new DispatcherTimer();
        _timer.Interval = new TimeSpan(0, 0, interval);
        _timer.Tick += timer_Tick;
    }

    void timer_Tick(object sender, EventArgs e)
    {
        this.Seconds += 1;
        if (this.Seconds == 60)
        {
            this.Minutes += 1;
            this.Seconds = 0;
        }
        if (this.Minutes == 60)
        {
            this.Hours += 1;
            this.Minutes = 0;
        }
    }

    public void Start()
    {
        _timer.Start();
    }

    public void Stop()
    {
        _timer.Stop();


    }
    public void Reset()
    {
        _timer = new DispatcherTimer();
        this.Seconds = 0;
        this.Minutes = 0;
        this.Hours = 0;
        this.Splits.Clear();
    }

    public void Split()
    {
        var timeSpan = new TimeSpan(this.Hours, this.Minutes, this.Seconds);
        this.Splits.Add(timeSpan);
    }

    public void Dispose()
    {
        _timer.Tick -= timer_Tick;
    }
}

And the WP8 button code (secondsValue is a text block on the wp8 page).

private async void startStopwatch_Click(object sender, RoutedEventArgs e)
    {
        watch.Start();


        await Task.Factory.StartNew(() =>
        {
            while (watch.IsRunning)
            {
                Thread.Sleep(1000);
                Dispatcher.BeginInvoke(() => secondsValue.Text = watch.Seconds.ToString());
            }

        });
    }
\$\endgroup\$
3
\$\begingroup\$

Minor

  1. You should get rid of the Class suffix. There is no value in attaching the type as a suffix to the name.

  2. You don't need to write this everywhere to access members.

  3. You have an Elapsed property which is not used.

Bugs/Major

  1. Your Reset() method will break the stopwatch because you create a new timer but you do not re-attach the timer tick event handler.

  2. You have a Reset() method which sets up the timer but you also do this in the constructor which is some code duplication and in fact has introduced a bug (see previous point). DRY - Don't Repeat Yourself. Make use of a common method which can get called in both places.

  3. Instead of "polling" your stopwatch in a continuous loop your StopwatchClass should expose an event like TimeUpdated to which any interested party can hook up to.

    In general this can pose problems when interacting with UI because if the callback happens on a different thread then you need to marshal UI access across to the UI thread. However you have used a DispatcherTimer which is evaluated at the top of every dispatcher loop - which means the callbacks are happening on the UI thread and it is safe to access UI elements from within the callback (see the MSDN example).

    So basically you can expose an event which gets fired on the timer callback and whatever code hooks up to it can update the UI.

  4. In general MVVM has established itself as a very useful pattern. Which means that rather than updating UI elements directly you would update an observable view model to which the UI is bound. This decouples the UI better from the data and decoupling is always good.

  5. The timer is guaranteed to not fire before the interval has elapsed - but there is no guarantee about how long after the interval has elapsed it will fire. This means your stopwatch will start falling behind the real time as you assume each tick is 1 second while in fact it might be 1.01 sec or so. This means after 100 seconds you could be easily 1 sec off or more. A thread time slice in windows is typically around 10ms so I'd suspect the timer to be around that accuracy. Although on Windows Phone you might get better performance (couldn't find any documentation in that regards).

    The way to overcome this is to take a timestamp at Start and compute the difference to the current time on each tick. While this will probably also give you an approx. 10ms accuracy it will be constant while your solution will accumulate an error on each tick.

    This will also alleviate the need to compute the seconds, minutes and hours since passing as you can get that from the Timespan.

With the changes from above your code could look like this:

public class StopwatchClass : IDisposable
{
    DispatcherTimer _timer;

    public int Interval { get; set; }

    public int Seconds { get; set; }
    public int Minutes { get; set; }
    public int Hours { get; set; }

    public bool IsRunning { get { return _timer.IsEnabled; } }
    public TimeSpan Elapsed { get; set; }
    public List<TimeSpan> Splits { get; set; }

    private DateTime _StartTimestamp = DateTime.MaxValue;

    public event EventHandler TimeUpdated;
    private void OnTimeUpdated()
    {
        var handler = TimeUpdated;
        if (handler != null)
        {
            TimeUpdated(this, EventArgs.Empty);
        }
    }

    public StopwatchClass(int interval)
    {
        Reset();
    }

    void timer_Tick(object sender, EventArgs e)
    {
        var timeSinceStart = DateTime.Now - _StartTimestamp;
        Seconds = (int)timeSinceStart.Seconds;
        Minutes = (int).timeSinceStart.Minutes;
        Hours = (int)timeSinceStart.TotalHours;
        OnTimeUpdated();
    }

    public void Start()
    {
        _StartTimestamp = DateTime.Now;
        _timer.Start();
    }

    public void Stop()
    {
        _StartTimestamp = DateTime.MaxValue
        _timer.Stop();
    }

    public void Reset()
    {
        Seconds = 0;
        Minutes = 0;
        Hours = 0;
        Splits = new List<TimeSpan>();
        _timer = new DispatcherTimer();
        _timer.Interval = new TimeSpan(0, 0, interval);
        _timer.Tick += timer_Tick;
    }

    public void Split()
    {
        var timeSpan = new TimeSpan(Hours, Minutes, Seconds);
        Splits.Add(timeSpan);
    }

    public void Dispose()
    {
        _timer.Tick -= timer_Tick;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ so to clarify re the MVVM approach, the ViewModel would have a property (say, elapsed) that is being updated by the stopwatch class? \$\endgroup\$ – Adam Grande Feb 6 '14 at 4:39
  • \$\begingroup\$ @AdamGrande: Yes that's the basic idea. \$\endgroup\$ – ChrisWue Feb 10 '14 at 23:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.