Stopwatch exercise from training course Github

Github for easy testing



I'm currently undertaking a training course to try &amp; further develop my skills in 

C#. The latest exercise was to create a basic stopwatch class that meets the following criteria -


  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.
  
  Educational tip: The aim of this exercise is to make you understand
  that a class should be always in a valid state. We use encapsulation
  and information hiding to achieve that. The class should not reveal
  its implementation detail. It only reveals a little bit, like a
  blackbox. From the outside, you should not be able to misuse a class
  because you shouldn’t be able to see the implementation detail.


I'm sure someone is bound to mention it, but yes I'm aware that there is already a stopwatch class in the .NET framework, but as this was the exercise I wanted to try &amp; accomplish it based on the requirements.

The class is as follows -

public class Stopwatch
{
    private TimeSpan _duration;
    // private TimeSpan _start;
    private DateTime _start;

    public Stopwatch()
    {
        ZeroStart();
    }

    public void Start()
    {
        // if (_start != TimeSpan.Zero)
        if (_start != DateTime.Min)
            throw new InvalidOperationException("The stopwatch has already been started.");

        _start = DateTime.Now;
    }

    public TimeSpan Stop()
    {
        // if (_start == TimeSpan.Zero)
        if (_start == DateTime.Min)
            throw new InvalidOperationException("The stopwatch has not been started.");

        _duration = DateTime.Now- _start;
        ZeroStart();
        return _duration;
    }

    private void ZeroStart()
    {
        // _start = TimeSpan.Zero;
        _start = DateTime.Min;
    }
}


I haven't done an awful lot of OOP or even effective work with classes. Most of the stuff I have done in the past has been more procedural based stuff, just a long list of static methods in static classes etc...

I'm not sure if this is really enough to go on for anyone to actually critique me on so I'm sorry if that's the case.

I've tested the code using the following -

class Program
{
    static void Main(string[] args)
    {
        UsingStopwatch();
    }

    static void UsingStopwatch()
    {
        Console.WriteLine("This is a stop watch. Type 'start' to start it &amp; 'stop' to stop it.");
        var stopwatch = new Stopwatch();

        while (true)
        {
            var input = Console.ReadLine();

            switch (input.ToLower())
            {
                case "start":
                    stopwatch.Start();
                    break;
                case "stop":
                    Console.WriteLine(stopwatch.Stop());
                    break;
                default:
                    Console.WriteLine("Sorry I don't recognize that.");
                    break;
            }
        }
    }
}




Update

Thanks to jandotnet I have made some updates to how I store some of the data. _start is now DateTime &amp; I have comparing values with DateTime.MinValue. I have updated the above with the full snippet.
I see no problem with a programming challenge that has a low accuracy stopwatch.  I have problems with your particular implementation.  Since one aim of exercise is to be fully aware of state, I am surprised in how it maintains state indirectly and privately based on the value in _start.  A simple stopwatch has a very simple state: its either running or not.  So I would just directly offer a Boolean IsRunning property with public getter and private setter.  This removes any mystery as to what the state is, and it exposes that state clearly and publicly to the consumer of the stopwatch.

I personally would prefer _start to be renamed _startTime, but that's no big deal.  I would have the Start() method do 2 things: (1) sets IsRunning to true, and (2) sets _startTime to DateTime.UtcNow.  Note that UtcNow is not only faster than Now (Now actually calls UtcNow first), but also allows the stopwatch to not have any odd side effects from Daylight Saving Time transitions (really the biggest reason to use UtcNow).

My other big gripe is the Stop() method should just stop the stopwatch.  Your implementation has it stopping and also returning the duration.  I would change Stop() to be void, and then offer a TimeSpan Duration property.  Stop() should set IsRunning to false, and somehow store the duration for later retrieval.

This now gives you flexibility that you did not possess before.  What behavior do you want the stopwatch to exhibit if someone asks for the Duration while its running?  Do you want to throw an exception because Stop() has not been issued first?  Or would you like to return a Duration while running?