11
\$\begingroup\$

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;
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Given the beginner tag, I strongly suspect the answer is "no", but: is your code required to be thread-safe? \$\endgroup\$ – Philip Kendall Mar 15 '18 at 16:03
  • 1
    \$\begingroup\$ I have a question about the requirements - they say that you should be able to use a stopwatch multiple times (start, stop, start, stop) but they don't say what they behaviour should be on a restart. Your code will reset the counter on a restart, but my intuitive expectation of a stopwatch would be that if you restart a stopped stopwatch then it should continue from where it left off. \$\endgroup\$ – Pete Mar 15 '18 at 16:36
  • 1
    \$\begingroup\$ @Pete But then I would expect a reset() method in addition to start() and stop(). \$\endgroup\$ – BlackJack Mar 15 '18 at 17:17
10
\$\begingroup\$

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.

\$\endgroup\$
  • 10
    \$\begingroup\$ Wouldn't using System.Diagnostics.StopWatch defeat the purpose of the exercise? \$\endgroup\$ – JAD Mar 15 '18 at 13:31
  • 2
    \$\begingroup\$ @JAD perhaps, but I always like to point it out whenever anyone is timing anything. I interpreted the exercise as being more about implementing a spec than providing functionality: the task is to get the API spot-on: System.Diagnostics.Stopwatch doesn't throw is you call Start()twice, nor does Start() reset: it would just be providing the time-keeping, nothing more. \$\endgroup\$ – VisualMelon Mar 15 '18 at 13:39
  • \$\begingroup\$ FYI ... I added reinventing-the-wheel as a tag after your answer was posted. \$\endgroup\$ – Rick Davin Mar 15 '18 at 19:43
  • 1
    \$\begingroup\$ The inline-documentation also knows an explicit exception Tag that you can use - with this, you also get a reference to the exception type: /// <exception cref="InvalidOperationException">Thrown if the Stopwatch is already running.</exception> \$\endgroup\$ – Dennis Mar 16 '18 at 13:13
10
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ "There's no need to underbar private fields in C#. Lots of people do. It looks weird to me." Well, it signals state to a reviewer when (this.) is (correctly) not present. It's also in the style guidelines of both the C# compiler team and the .NET BCL, so most of the official examples use them. (We don't use the m, though, since it's redundant with the _) \$\endgroup\$ – bartonjs Mar 16 '18 at 17:10
  • \$\begingroup\$ @bartonjs: Sure, opinions vary. It's just a super weird thing to call out as a naming convention something that could be determined by looking at the declaration. I can think of a great many properties that I would call out in a naming convention before something that could be determined so easily. \$\endgroup\$ – Eric Lippert Mar 16 '18 at 17:15
  • \$\begingroup\$ Obligatory note to the asker that what the convention is isn’t important. Being consistent is. \$\endgroup\$ – RubberDuck Mar 18 '18 at 10:55
9
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The problem description is clearly a simple example, not a professional request. Given the simplicity of the problem description, using DateTime seems to fit the expectation of whoever wrote the description (because otherwise OP could simply use the StopWatch class). What you say is corerct, but not necessarily relevant for OP's current assignment. \$\endgroup\$ – Flater Mar 15 '18 at 15:18
  • 1
    \$\begingroup\$ I like your TickCount answer. If OP sticks with a DateTime, I suggest he use DateTime.UtcNow. \$\endgroup\$ – Rick Davin Mar 15 '18 at 19:45
  • \$\begingroup\$ You'd better not run it longer than 24.9 days then: "Because the value of the TickCount property value is a 32-bit signed integer, if the system runs continuously, TickCount will increment from zero to Int32.MaxValue for approximately 24.9 days, then jump to Int32.MinValue, which is a negative number, then increment back to zero during the next 24.9 days. You can work around this issue by calling the Windows GetTickCount function, which resets to zero after approximately 49.7 days, or by calling the GetTickCount64 function." \$\endgroup\$ – RobIII Mar 16 '18 at 0:14
  • 1
    \$\begingroup\$ There are other problems with GetTickCount, for example, you start the stopwatch, you hibernate your machine, you thaw it out five minutes later -- are we guaranteed that the current tick count is five minutes larger than it was when we hibernated the machine? Not a rhetorical question; I genuinely have no idea. But I would want to know before I relied on that. OS features like hibernation impose heavy taxes that even simple applications must pay. \$\endgroup\$ – Eric Lippert Mar 16 '18 at 0:28
  • \$\begingroup\$ And @RobIII's problem is worse than they've said. The problem is not "the process runs for 25 days". The problem is the program runs such that the stopwatch start and stop straddle an overflow. If you're making a lot of stopwatches all the time, and your machine has more than a few weeks of uptime, odds are good you'll hit this eventually, even if the individual stopwatch durations are short. \$\endgroup\$ – Eric Lippert Mar 16 '18 at 0:30
4
\$\begingroup\$

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.

\$\endgroup\$

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.