DebugTimer design

Question

Summary:

• I created this DebugTimer class so that I could test performance of code in a much cleaner/shorter syntax.
• Prior I was using regular timers and wrapping them all in #if DEBUG #endif.
• I'm relatively new to events, have I done them well?
• This is the first time planning a snippet of code to be used externally (what I mean by that is I made DebugTimer.cs as its own library-project and Debugger.cs is what would be used in the project to time things, my attempt at de-coupling).

Advice Desired...

• Thoughts on the DEBUG mode only part? Originally I was just going to wrap most of the code in #if DEBUG #endif but this proved problematic when planning it as an external library.dll since the DEBUG part listened to how I built the library and not the project it was being used in.
• Have the events been well formed? Should I add/remove any that may or may not be desirable?
• I made Timer a private class with a public ITimer interface. Looking back I'm not really sure why I did it, I think my thought process was to force the use of DebugTimer instead of creating an instance of Timer and using that (Whether that's a good reason or not is beyond me).

DebugTimer.cs

public delegate void DebugTimerHandler(ITimer sender, String TimerName);
/// <summary>
/// Allows for Debug-Mode timing with minimal lines of code required for use.
/// 1) Subscribe to the OnDebugTimerStopped and OnDebugTimerStarted events to log information.
/// 2) Use StartTimer supplying a Unique name to start a new timer.
/// 3) Use StopTimer supplying an existing timer's name to stop the timer.
/// Only executes code if built in Debug-Mode.
/// </summary>
public class DebugTimer
{
 private Boolean DebugMode { get; set; }
 private Dictionary<String, ITimer> _Timers { get; set; }
 private DebugTimerHandler _OnDebugTimerStopped { get; set; }
 private DebugTimerHandler _OnDebugTimerStarted { get; set; }
 /// <summary>
 /// A timer that only executes if ran in Debug-Mode. For external library reasons,
 /// this must be passed in as a variable (otherwise it detects if the library itself was
 /// built in debug/release mode.
 /// See here for more information: 
 /// http://stackoverflow.com/questions/654450/programmatically-detecting-release-debug-mode-net
 /// </summary>
 /// <param name="DebugMode">Use #if DEBUG #endif to detect if you're in Debug-Mode.</param>
 /// <param name="OnDebugTimerStarted">Event executed right before the timer starts.</param>
 /// <param name="OnDebugTimerStopped">Event executed right after the timer stops.</param>
 public DebugTimer(Boolean DebugMode = true, DebugTimerHandler OnDebugTimerStarted = null, DebugTimerHandler OnDebugTimerStopped = null)
 {
 this.DebugMode = DebugMode;
 if (DebugMode)
 {
 this._Timers = new Dictionary<String, ITimer>();
 this._OnDebugTimerStarted = OnDebugTimerStarted ?? delegate { };
 this._OnDebugTimerStopped = OnDebugTimerStopped ?? delegate { };
 }
 }
 /// <summary>
 /// Does nothing if not compiled in Debug-Mode.
 /// </summary>
 public void StartTimer(String Timer)
 {
 if (DebugMode)
 {
 Timer timer = new Timer(Timer);
 timer.DebugTimerStarted += this._OnDebugTimerStarted;
 timer.DebugTimerStopped += this._OnDebugTimerStopped;
 if (_Timers.ContainsKey(Timer))
 {
 _Timers[Timer].Start();
 //throw new Exception(String.Format("DebugTimer with the name {0} already exists!", Timer));
 }
 else
 {
 _Timers.Add(Timer, timer);
 _Timers[Timer].Start();
 }
 }
 }
 /// <summary>
 /// Does nothing if not compiled in Debug-Mode.
 /// </summary>
 public void StopTimer(String Timer)
 {
 if (DebugMode)
 {
 if (_Timers.ContainsKey(Timer))
 {
 _Timers[Timer].Stop();
 }
 }
 }
 private class Timer : ITimer
 {
 private String Name { get; set; }
 private Stopwatch _Stopwatch { get; set; }
 public long Miliseconds
 {
 get
 {
 return _Stopwatch.ElapsedMilliseconds;
 }
 }
 public long Ticks
 {
 get
 {
 return _Stopwatch.ElapsedTicks;
 }
 }
 #region Events
 public event DebugTimerHandler DebugTimerStopped;
 protected virtual void OnDebugTimerStopped(String TimerName)
 {
 if (DebugTimerStopped != null)
 DebugTimerStopped(this, TimerName);
 }
 public event DebugTimerHandler DebugTimerStarted;
 protected virtual void OnDebugTimerStarted(String TimerName)
 {
 if (DebugTimerStarted != null)
 DebugTimerStarted(this, TimerName);
 }
 #endregion
 public Timer(String Name)
 {
 this.Name = Name;
 this._Stopwatch = new Stopwatch();
 }
 public void Start()
 {
 _Stopwatch.Reset();
 OnDebugTimerStarted(this.Name);
 _Stopwatch.Start();
 }
 public void Stop()
 {
 _Stopwatch.Stop();
 OnDebugTimerStopped(this.Name);
 }
 }
}

Debugger.cs:

public static class Debugger
{
 #if DEBUG
 static Boolean DebugMode = true;
 #else
 static Boolean DebugMode = false;
 #endif
 public static DebugTimer Timer = new DebugTimer(DebugMode, PreLog, PostLog);
 private static void PreLog(ITimer sender, String TimerName)
 {
 Console.WriteLine("{0}...", TimerName);
 }
 private static void PostLog(ITimer sender, String TimerName)
 {
 Console.WriteLine("Milliseconds: {1} Ticks: {2}\n...{0}", TimerName, sender.Miliseconds, sender.Ticks);
 }
}

Usage example:

Debugger.Timer.StartTimer("Testing");
for (int c = 0; c < 10; c++)
{
 Debugger.Timer.StartTimer("++i");
 for (int i = 0; i < 1000000; ++i)
 {
 }
 Debugger.Timer.StopTimer("++i");
 Debugger.Timer.StartTimer("i++");
 for (int i = 0; i < 1000000; i++)
 {
 }
 Debugger.Timer.StopTimer("i++");
 Console.WriteLine();
}
Debugger.Timer.StopTimer("Testing");
Console.ReadKey();

Answer 1

Based on the naming guidelines input parameters should be named using camelCase casing.

public DebugTimer(Boolean DebugMode = true, DebugTimerHandler OnDebugTimerStarted = null, DebugTimerHandler OnDebugTimerStopped = null)

should be

public DebugTimer(Boolean debugMode = true, DebugTimerHandler onDebugTimerStarted = null, DebugTimerHandler onDebugTimerStopped = null)

this

public delegate void DebugTimerHandler(ITimer sender, String TimerName); 

should be

public delegate void DebugTimerHandler(ITimer sender, String timerName); 

Propertynames shouldn't start with an underscore _ either. This should be used for classmember variables only. One would either use _someVariable, mSomeVariable or just someVariable for a classmember variable, but always SomeVariable for a property name.

---

Sometimes you are using this. to refer to a class property, sometimes you aren't. You should stick to one style and not mixing them.

---

Using private properties (getter and setter are private) is only adding noise to the code. There isn't a real benefit of using them if you hadn't some validation logic etc. in the setter.

---

public void StartTimer(String Timer)
{
 if (DebugMode)
 {
 Timer timer = new Timer(Timer);

this just smells. Using a variable name which is the same as an existing,used class.

---

Commented code like //throw new Exception(String.Format("DebugTimer... is dead code which only adds noise and should be deleted.

---

By adding a guard clause like

if (!DebugMode) { return; } 

you will reduce horizontal spaceing and therefor add readability.

---

public void StartTimer(String Timer)
{
 if (DebugMode)
 {
 Timer timer = new Timer(Timer);
 timer.DebugTimerStarted += this._OnDebugTimerStarted;
 timer.DebugTimerStopped += this._OnDebugTimerStopped;
 if (_Timers.ContainsKey(Timer))
 {
 _Timers[Timer].Start();
 //throw new Exception(String.Format("DebugTimer with the name {0} already exists!", Timer));
 }
 else
 {
 _Timers.Add(Timer, timer);
 _Timers[Timer].Start();
 }
 }
}

Instead of creating the Timer object first and then checking if the name of the timer is present in the dictionary, you should create the timer if it does not exists and the start it.

public void StartTimer(String timerName)
{
 if (!DebugMode) { return; }
 if (!_Timers.ContainsKey(timerName))
 {
 Timer timer = new Timer(timerName);
 timer.DebugTimerStarted += this._OnDebugTimerStarted;
 timer.DebugTimerStopped += this._OnDebugTimerStopped;
 _Timers.Add(timerName, timer); 
 }
 _Timers[timerName].Start();
} 

---

You should use braces for single statements if statements too which makes your code less error prone.

If you decide to not use braces in these cases, you should stick the the choosen style. Right now you are mixing the styles.

---

Timing methods will only be accurate, if compiled in release mode.

See also my answer here: https://codereview.stackexchange.com/a/80051/29371 about ProcessorAffinity and PriorityClass of the current process.

---

Usually an event has a signature like (object sender... instead of (ITimer sender... but this isn't a issue.

---

The private vs. public scope of the Timer class is a little bit more tricky. This is mostly because DebugTimer is only pretending by its name to be a timer but is in reality only holding a collection of different timers.

A better (and more OO) approach would be to create a DebugTimer class which implements ITimer and uses the Timer class by composition. This DebugTimer class should contain the DebugMode property, so you also could add a ReleaseTimer class without this property.

Both timer classes could then be contained in a Timing class which holds a Dictionary<string, ITimer> and having a AddTimer() and RemoveTimer() method.

• Shelby115: I am really-really bad at pascal-casing parameters haha. I never learned the C# naming conventions until I had already used it a significant amount haha. I try. Thanks for the feedback, it is helpful. I was aware of the whole object sender part but wasn't really sure if I was supposed to keep it object or make it ITimer since it was the only sender I was going to be using for that delegate/event.