2
\$\begingroup\$

Github

Github for easy testing


I'm currently undertaking a training course to try & 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 & 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 & '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 & I have comparing values with DateTime.MinValue. I have updated the above with the full snippet.

\$\endgroup\$
  • 1
    \$\begingroup\$ Using TimeOfDay is not is good idea if you want to use the stop watch from one day to the next ;). i.e: new DateTime(2019, 01, 02, 00, 05, 00).TimeOfDay - new DateTime(2019, 01, 01, 23, 55, 00).TimeOfDay -> -23:50:00. You can just use DateTime instead. \$\endgroup\$ – JanDotNet Apr 11 at 20:15
  • \$\begingroup\$ Sorry if this sounds stupid but if I change it to _start = DateTime; or _start = new DateTime; both throw exceptions. The former being because DateTime is a type & the latter being that DateTime can't be converted to TimeSpan. How exactly do you mean to just use DateTime? \$\endgroup\$ – Webbarr Apr 11 at 20:35
  • 1
    \$\begingroup\$ My suggestion is: change type of _start to DateTime and use DateTime.Now instead of DateTime.Now.TimeOfDate. \$\endgroup\$ – JanDotNet Apr 12 at 4:09
  • \$\begingroup\$ Only way to get your Stopwatch as accurate as the built in stopwatch is to use the OS tick docs.microsoft.com/en-us/windows/desktop/api/sysinfoapi/… \$\endgroup\$ – Anders Apr 12 at 9:13
  • \$\begingroup\$ @JanDotNet Thank you for clarifying, sorry I didn't understand at first. \$\endgroup\$ – Webbarr Apr 12 at 15:50
3
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for your feedback! Your answer makes perfect sense. I've updated my GitHub repo with your suggested changes & I'm a lot happier with it. My Stop() method was one thing that I wasn't particulary happy with, your comment about the name of _start to _startTime just makes more sense in my head. I'm a bit annoyed at myself for not thinking about the state of the object itself. Thank you for teaching me about public getters & private setters (I honestly had no clue that was a thing). \$\endgroup\$ – Webbarr Apr 19 at 11:02
  • 1
    \$\begingroup\$ @Webbarr I peeked at your updated GitHub. Amazingly how short and simple the new code base is! Much cleaner to read and understand. There is a bug in Stop (line 26) where you set IsRunning to true when it should be set to false. \$\endgroup\$ – Rick Davin Apr 19 at 12:40
  • \$\begingroup\$ it is! Quite embarrassing that I missed that. I've fixed it now. Thank you again for your time, I think the main thing I need to take away is to stop rushing it (missing a glaringly obvious bug like that as a prime example) \$\endgroup\$ – Webbarr Apr 19 at 20:29

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.