3
\$\begingroup\$

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.

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.

That is the information given to me from the tutorial I am taking to teach myself C#. I have completed this and looking for ways to improve my code and/or learn something new here. I have tested this and to my knowledge is working as expected. Any help is appreciated.

using System;

namespace ExerciseOne
{
    public static class Stopwatch
    {
        private static DateTime TimeStart { get; set; }
        private static DateTime TimeStop { get; set; }
        private static bool isStarted = false;
        private static bool isStopped = false;

        private static void StartTimer(DateTime start)
        {
            if (isStarted)
            {
                throw new InvalidOperationException("Unable to start a stopwatch twice in a row.");
            }
            else
            {
                isStarted = true;
                isStopped = false;
                TimeStart = start;
            }
        }

        private static void StopTimer(DateTime stop)
        {
            if (isStopped)
            {
                throw new InvalidOperationException("Unable to stop a stopwatch twice in a row.");
            }
            else
            {
                isStarted = false;
                isStopped = true;
                TimeStop = stop;
            }
        }

        private static string ElapsedTimer() => (TimeStop - TimeStart).ToString();

        private static void Begin()
        {
            StartTimer(DateTime.Now);
            System.Console.WriteLine(" - Stopwatch has begun.");
        }
        private static void End()
        {
            StopTimer(DateTime.Now);
            Console.WriteLine($" - Stopwatch has stopped. Elapsed Time: {ElapsedTimer()}");
        }

        public static void RunProgram()
        {
            Console.WriteLine("Stopwatch program.");
            Console.WriteLine("Type \"S\" to start the program. Type \"T\" to stop the program. Type \"E\" to end the program.");

            while (true)
            {
                ConsoleKeyInfo cki = Console.ReadKey(false);

                if (cki.Key == ConsoleKey.E)
                {
                    Console.WriteLine(": \"E\" key was pressed.  Progam exited.");
                    return;
                }
                else if (cki.Key == ConsoleKey.S)
                {
                    Begin();
                }
                else if (cki.Key == ConsoleKey.T)
                {
                    End();
                }
                else
                {
                    Console.WriteLine("\nPlease type either \"S\" to start the program, \"T\" to stop the program, \"E\" to end the program.");
                }
            }
        }
    }
}
using System;

namespace ExerciseOne
{
    class Program
    {
        static void Main() => Stopwatch.RunProgram();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Related question that I once answered. See OP for GitHub link. codereview.stackexchange.com/questions/217285/… \$\endgroup\$ – Rick Davin Mar 17 at 14:23
  • 1
    \$\begingroup\$ Nice that you try things on your own. If you ever want to see how Microsoft does it, check out referencesource.microsoft.com/#System/services/monitoring/… \$\endgroup\$ – Rick Davin Mar 17 at 14:25
  • \$\begingroup\$ Please be aware that Stopwatch is commonly used as a high resolution timer. The answers I see here do not address that issue. If you don't care, you can use the DateTime class and properties, but be aware they will be nowhere near as accurate as Stopwatch. If you need a high resolution example I can write an answer. \$\endgroup\$ – Zer0 Mar 17 at 16:50
  • 1
    \$\begingroup\$ @Zer0 This is taking from a coding exercise, in which case the learners do not need a high resolution timer. The emphasis is on class design, not timers. If they needed a high resolution timer in some real application, they would obviously use System.Diagnostics.Stopwatch. \$\endgroup\$ – Rick Davin Mar 17 at 16:57
  • \$\begingroup\$ @RickDavin Just noting the difference. And that you can code a high resolution timer without Stopwatch. \$\endgroup\$ – Zer0 Mar 17 at 17:01
6
\$\begingroup\$
public static void RunProgram()
{
   ....
}

Why do you run the application as a method on the Stopwatch class it self?


You have made all the methods (StartTimer(), StopTimer()) private. This means that they only can be run from your RunProgram method, which occupy the main thread of the program and measures nothing. This set up makes the entire effort rather useless. I would expect a stopwatch being able to measure something in a way like:

  Stopwatch stopwatch = Stopwatch.StartNew();

  // TODO: execute something you want to measure
  Thread.Sleep(5000);

  stopwatch.Stop();

  Console.WriteLine($"Duration: {stopwatch.Duration}");

    private static bool isStarted = false;
    private static bool isStopped = false;

These flags can be combined into one isRunning, used like:

    private static void StartTimer(DateTime start)
    {
        if (isRunning)
        {
            throw new InvalidOperationException("Unable to start a stopwatch twice in a row.");
        }
        else
        {
            isRunning = true;
            TimeStart = start;
        }
    }

and

    private static void StopTimer(DateTime stop)
    {
        if (!isRunning)
        {
            throw new InvalidOperationException("Unable to stop the stopwatch because it is not running.");
        }
        else
        {
            isRunning = false;
            TimeStop = stop;
        }
    }

    private static void Begin()
    {
        StartTimer(DateTime.Now);
        System.Console.WriteLine(" - Stopwatch has begun.");
    }

You shouldn't write to the console in class methods of any object, unless it's explicitly meant to be a console application. The above message is useless in a WinForm application.


private static void StartTimer(DateTime start)

I don't se why you have a start time as argument to StartTimer. Why not just call DateTime.Now inside it? And likewise in StopTimer.


You were told to provide the result as a TimeSpan, but you actually return a string:

private static string ElapsedTimer() => (TimeStop - TimeStart).ToString();


public static class Stopwatch

The major problem with your implementation is though, that you make it static. That means that you only can run one "instance" at a time. In this way you aren't able to measure on two treads at the same time - or have nested measurements.

I would remove all the static stuff and only have one static method - starting a new Stopwatch instance, that then can be stopped by calling Stop() on the returned object like I showed above. So the public interface of the object would be:

  public class Stopwatch
  {
    public TimeSpan Duration { get; }

    public void Start() { }
    public void Stop() { }
    public static Stopwatch StartNew() { }
  }

I think you have misunderstood the concepts of "encapsulation and information hiding" slightly. An object must have a public interface through which clients can communicate with it, but they shouldn't be allowed to manipulate the objects internal state (for instance the StartTime member of your watch) - only through the public interface. The client shouldn't know about how you measure the time internally, they are only interested in the final result, when they stop the watch by calling Stop().

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Best practices for internal time keeping is to use DateTime.UtcNow instead of DateTime.Now. \$\endgroup\$ – Rick Davin Mar 17 at 14:19
  • \$\begingroup\$ I think you have misunderstod the concepts of "encapsulation and information hiding" slightly. Indeed, I started this lesson thinking to myself that I don't understand the concept and just tried my best. \$\endgroup\$ – Milliorn Mar 17 at 22:59
  • \$\begingroup\$ Based on all you commented on I am working on a refactor to the best of my understanding of your suggestions and will post it when I am done. Thank you for taking the time to point out how to improve this code. \$\endgroup\$ – Milliorn Mar 17 at 23:18
  • \$\begingroup\$ This is my refactor so far. gist.github.com/milliorn/96d009b2a17f7ff05e884057851f0089 \$\endgroup\$ – Milliorn Mar 18 at 0:12
  • 1
    \$\begingroup\$ @Henrik Hansen it took me about an hour to read over both post here a few times and really think about what was said. I was getting confused at first and frustrated. But after an hour of looking at my code and reading everything over a few times it started to make sense what was suggested and why. The end result is definitely better than where I started. I appreciate all your help so far on my post and thank you for it. \$\endgroup\$ – Milliorn Mar 18 at 7:19
2
\$\begingroup\$

I agree with @Henrik_Hansen's answer. My answer builds upon his.

I absolutely don't see a reason for anything to be static.

Your Stopwatch class should only be for a stop watch. Right now you have commingled a UI into the stopwatch. In the name of Separation of Concerns, I see no reason for any Console.Write or Console.WriteLine in the Stopwatch class.

Going further, I see no reason for any method in Stopwatch to return a string. For Duration, simply return the TimeSpan value and leave it to whoever called that to decide what to do with it. Maybe they want it ToString(), but that's up to them. Maybe they want to add the Duration leg to cumulative TimeSpan.

Any internal time keeping calling DateTime should always use DateTime.UtcNow and not DateTime.Now. Granted UtcNow is faster than Now but in real-time that is laughably negligible. The main reason to do it is to avoid any Daylight Saving Time transitions where you start the stopwatch during standard time and stop it during DST. This would produce incorrect Duration.

A stopwatch really has only 1 state. It is either running or it is stopped. You do not need 2 variables for start and stop state. This runs the risk of you accidentally having conflicting states, e.g. the isStarted and isStopped are both true or both false. You only need 1 such variable. I personally prefer this:

public bool IsRunning { get; private set; }

Adding to Henrik's skeletal class, I would update it to:

public class Stopwatch
  {
    public TimeSpan Duration { get; }
    public bool IsRunning { get; private set; }

    public void Start() { }
    public void Stop() { }
    public static Stopwatch StartNew() { }
  }

The interesting thing you can do now with Duration is control the behavior of what happens if someone asks for the Duration while the stopwatch is running. Would you want to throw an exception? I personally would want to see the current Duration without stopping the stopwatch. This would produce a leg time while letting the stopwatch continue running.

Once you get this going, I would suggest you could add other nice methods. This would mirror Diagnostics.Stopwatch. Two such methods would be Reset and Restart.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Good point about using UTC in case of Daylight Saving Time transition - very important. The OP does not state the level of precision sought - the strategies will be different depending on whether we want accuracy to the second, millisecond or microsecond. Plus, it should be possible to query the duration while the stopwatch is still running. Perhaps invoke a callback function at regular intervals and leave the console handling to a delegate. \$\endgroup\$ – Anonymous Mar 17 at 17:34
  • \$\begingroup\$ @Rick I have been unable to figure out how to do this suggestion Going further, I see no reason for any method in Stopwatch to return a string. Your wiki link was very informative and useful. I did change to DateTime.UtcNow. Good catch. Based on Henrik and your post I was able to refactor my code to this suggestion here A stopwatch really has only 1 state. It is either running or it is stopped. Thank you both for this. \$\endgroup\$ – Milliorn Mar 18 at 0:06
  • \$\begingroup\$ This is my refactor so far. gist.github.com/milliorn/96d009b2a17f7ff05e884057851f0089 \$\endgroup\$ – Milliorn Mar 18 at 0:12
  • \$\begingroup\$ @Milliorn Avoiding strings in Stopwatch class would mean applying this change public TimeSpan ElapsedTimer() => (timeStop - timeStart); although you may need to check the is running state. \$\endgroup\$ – Rick Davin Mar 18 at 12:05
  • \$\begingroup\$ @RickDavin thought that was what I tried before posting. Obviously not since that works. Thank you for that. \$\endgroup\$ – Milliorn Mar 18 at 21:18
1
\$\begingroup\$

This is my refactor based on all the suggestions made here. Works quite nice. Thank you all for your help and teaching me a few things.

Stopwatch.cs

using System;

namespace ExerciseOne
{
    public class Stopwatch
    {
        private DateTime timeStart;
        private DateTime timeStop;
        private bool isRunning;

        private void StartTimer()
        {
            if (isRunning)
            {
                throw new InvalidOperationException("Unable to start a stopwatch twice in a row.");
            }
            else
            {
                isRunning = true;
                timeStart = DateTime.UtcNow;
                timeStop = DateTime.UtcNow;
            }
        }

        private void StopTimer()
        {
            if (!isRunning)
            {
                throw new InvalidOperationException("Unable to stop a stopwatch twice in a row.");
            }
            else
            {
                isRunning = false;
                timeStop = DateTime.UtcNow;
            }
        }

        public TimeSpan ElapsedTimer => (timeStop - timeStart);
        public void Begin() => StartTimer();
        public void End() => StopTimer();
    }
}

Program.cs

using System;

namespace ExerciseOne
{
    class Program
    {
        static void Main()
        {
            Console.WriteLine("Stopwatch program.");
            Console.WriteLine("Type \"S\" to start the program. Type \"T\" to stop the program. Type \"E\" to end the program.");
            Stopwatch sw = new Stopwatch();

            while (true)
            {
                ConsoleKeyInfo cki = Console.ReadKey();

                if (cki.Key == ConsoleKey.E)
                {
                    Console.WriteLine(": \"E\" key was pressed.  Progam exited.");
                    return;
                }
                else if (cki.Key == ConsoleKey.T)
                {
                    sw.End();
                    Console.WriteLine($" - Stopwatch has stopped. Elapsed Time: {sw.ElapsedTimer}");
                }
                else if (cki.Key == ConsoleKey.S)
                {
                    sw.Begin();
                    System.Console.WriteLine(" - Stopwatch has begun.");
                }
                else
                {
                    Console.WriteLine("\nPlease type either \"S\" to start the program, \"T\" to stop the program, \"E\" to end the program.");
                }
            }
        }
    }
}
| improve this answer | |
\$\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.