Stopwatch class

Question

I am learning C# and I have an exercise:

Design a class called Stopwatch. The job of this class is to simulate a stopwatch. It should provide two methods: Start and Stop. We call the start method first, and the stop method next. Then we ask the stopwatch about the duration between start and stop. Duration should be a value in TimeSpan. Display the duration on the console. We should also be able to use a stopwatch multiple times. So we may start and stop it and then start and stop it again. Make sure the duration value each time is calculated properly. We should not be able to start a stopwatch twice in a row (because that may overwrite the initial start time). So the class should throw an InvalidOperationException if its started twice.

My code works fine, but please review if I could do something better. Especially I do not like this part with the switch case block but I dont have any idea how I could improve it.

Stopwatch class:

using System;

namespace Stopwatch
{
    public class Stopwatch
    {
        private DateTime _startDate;
        private DateTime _endDate;
        private bool _isRunning;

        public void Start()
        {
            if (_isRunning)
                throw new InvalidOperationException("Stopwatch is already running");

            _startDate = DateTime.Now;
            _isRunning = true;
        }

        public void Stop()
        {
            if (!_isRunning)
                throw new InvalidOperationException("Stopwatch is not running");

            _endDate = DateTime.Now;
            _isRunning = false;
        }

        public TimeSpan GetDuration()
        {
            return _endDate - _startDate;
        }

    }
}

Program class:

using System;

namespace Stopwatch
{
    class Program
    {
        static void Main(string[] args)
        {
            var stopWatch = new Stopwatch();

            while (true)
            {
                Console.WriteLine("Enter 'start' to start Stopwatch\nEnter 'stop' to end Stopwach\nEnter any key to exit:\n");
                var input = Console.ReadLine().ToLower();

                if (input == "start" || input == "stop")
                    UseStopwatch(stopWatch, input);
                else
                    return;
            }
        }

        static void UseStopwatch(Stopwatch stopWatch, string command)
        {
            switch (command)
            {
                case "start":
                    try
                    { stopWatch.Start(); }
                    catch (InvalidOperationException)
                    { Console.WriteLine("stopWatch is already started\n"); }
                    break;
                case "stop":
                    try
                    {
                        stopWatch.Stop();
                        Console.WriteLine("Duration: {0}\n", stopWatch.GetDuration());
                    }
                    catch (InvalidOperationException)
                    { Console.WriteLine("stopWatch is not started\n"); }
                    break;
                default:
                    break;
            }
        }
    }
}

Answer 1

First off, the obligatory recommendation that you use System.Diagnostics.StopWatch for this purpose, and not DateTime.Now (or even UtcNow, which won't go wrong if you happen to enter daylight savings while the program is running). Using Diagnostics.Stopwatch is more precise than the methods in DateTime, and it provides an Elapsed property which returns a TimeSpan.

---

GetDuration() is a bit odd, because if you call it before Stop(), then it will return nonsense. It should either throw, or perhaps compute the 'current' ellapsed time if the stopwatch is running. Either way, this should be documented (see below).

I'd also use a property for GetDuration (as Heslacher has suggested) unless it is going to throw (i.e. when Start() has been called but not Stop()), in which case that might ruffle a few feathers.

---

As usual, I'll recommend you add some inline-documentation (///) to these methods, which should explain when and why exceptions will be thrown (e.g. explain what calling Start() twice in a row does: from the name, I would be unsure whether it throws, does nothing, or restarts the timer).

/// <summary>
/// Starts the Stopwatch, resetting the elapsed time.
/// Throws an InvalidOperationException if the Stopwatch is already running.
/// </summary>
public static void Start()
{
    // snip
}

These don't take long to write, and can improve the API massively.

Answer 2

Good effort.

A few small points, with lots of links to my blog along the way.

namespace Stopwatch
{
    public class Stopwatch
    {

Never name a class the same as its namespace. A world of pain awaits those who do. My four part series of articles describing that world of pain begins here.

private DateTime _startDate;

There's no need to underbar private fields in C#. Lots of people do. It looks weird to me.

public void Start()
    {
        if (_isRunning)
            throw new InvalidOperationException("Stopwatch is already running");

You are given in the statement of the problem that Start is not idempotent. Expose a property for _isRunning so that the caller can tell whether it is legal to call Start right now. Never make the caller try-catch to see if what they are about to do is illegal. That's a vexing exception and it is a really bad design flaw.

    public void Stop()
    {
        if (!_isRunning)
            throw new InvalidOperationException("Stopwatch is not running");

You were not given in the statement of the problem that Stop is nonidempotent. It might be a reasonable choice to make Start and Stop have the same behaviour. Just be aware that it is a choice you've made, not a requirement of the spec.

    public TimeSpan GetDuration()
    {
        return _endDate - _startDate;
    }

This could reasonably be a getter-only property.

The duration is not computed properly if the sequence of events is Start, then GetDuration then Stop. The statement of the problem does not say what to do in this situation. What happens if you do that? Does it seem reasonable? Consider what might be a more reasonable behaviour; what does your intuition about real stopwatches tell you should be done here?

Similarly: what happens if GetDuration is called before Start ? Work out all the possible scenarios for all orderings of your three entrypoints and come up with a sensible specification for each. Then implement that specification and write some tests.

              try
                { stopWatch.Start(); }
                catch (InvalidOperationException)
                { Console.WriteLine("stopWatch is already started\n"); }

And now we see why vexing exceptions are so vexing. If Start and Stop are not idempotent then the right thing to do here should be:

if (!stopWatch.Running)
  stopWatch.Start();
else
  Console.WriteLine("stopWatch is already started\n");

Again never make your user catch an exception that tells them that they are doing something wrong. Either make it impossible for them to do something wrong, or provide some way of determining without a try-catch that the operation is disallowed.

           default:
                break;

Unnecessary. Delete it.

Additional exercises:

• Now implement a StopWatch that has operations IsRunning, Duration, Start, Pause, Resume and Stop. Again, work out all possible orderings of all possible operations and make sure that every operation has a sensible behaviour. Adding two more operations makes this a considerably harder problem. Consider studying finite state machine theory; this is the theory which underlies this sort of object.

• Stop using the console. Make a WPF or WinForms project that has Start / Stop etc buttons on a stopwatch control. Display the elapsed time in the control.

Answer 3

You have done well. You have created a class for the Stopwatch which only responsibility is just exactly what a Stopwatch should do : Start, Stop and calculate the duration.

What I don't like are the messages of the exceptions, not because they would be wrong or misleading, but because you show a different message in the catch block. You could easily change this like so

case "start":
    try
    { stopWatch.Start(); }
    catch (InvalidOperationException ex)
    { Console.WriteLine(ex.Message); }
    break;  

but if you would change it like so

case "start":
    try
    { 
        stopWatch.Start(); 
    }
    catch (InvalidOperationException ex)
    { 
        Console.WriteLine(ex.Message); 
    }
    break;  

it would be more readable on the first glance. Readability is a main point if it comes to maintain the code, meaning fixing bugs or adding features. You don't want to read the same code 3 times until you understand what it is about.

If you would let static void UseStopwatch() return a bool which you set to false in the default: case you could use the returned value as while condition. Like so

bool shouldRun = true;
while (shouldRun)
{



    shouldRun = UseStopwatch(stopWatch, input);
}  

in this way you could omit the if..else of the Main() .

Btw. catching the most specific exception, like you did, is the way to go.

---

What I would change about the Stopwatch is that I would use a property Duration instead of having a GetDuration() method.

Answer 4

You did actually very well, hovewer there's one major problem that would make your class unusable in production environment. Don't forget the DateTime.Now is a point in time that is changeable by user or other mechanisms you can't affect (e.g. DST change). So you can easily return invalid results which can be even negative - just try to move clock backwards while your stopwatch is running!

When measuring the duration never use difference of two DateTimes. If you can't use Stopwatch for any reason, stick with something that is not affected by calendar date, for example Environment.TickCount. Creating TimeSpan object from the difference of two TickCounts shouldn't be a problem for you.

Answer 5

Besides what was already mentioned in other answers, I'd like to point out the problem of not resetting _endDate when you start the timer.

It will be all good for the first run, but if you reuse the timer object, on the second start _endDate will still be set. Meaning any call to GetDuration will try to return a negative TimeSpan, since it will use a time from the past.