Code Smells

If you like practicing in identifying code smells, then you can find below a short class called TimerManager.

public class TimerManager
{
    public delegate void TimerCallback(object data);
    private static readonly object _sync = new object();
    private readonly Dictionary<int, Timer> timers = new Dictionary<int, Timer>();
    private readonly Dictionary<int, TimerCallback> callbacks = new Dictionary<int, 
        TimerCallback>();

     public void SetTimeout(TimerCallback timerCallback, int snooze)
     {
            var timer = new Timer(snooze);
            lock (_sync)
            {
               timers.Add(timer.GetHashCode(), timer);
               callbacks.Add(timer.GetHashCode(),
               x =>
               {
                  lock (_sync)
                  {
                     var t = timers[x.GetHashCode()];
                     t.Stop();
                     t.Close();
                     timers.Remove(x.GetHashCode());
                     callbacks.Remove(x.GetHashCode());
                  }
                  // invoke function provided by caller
                  timerCallback(null);
                });
            }
            timer.Elapsed += timer_Elapsed;
            timer.AutoReset = false;
            timer.Start();
      }
}

Here are some Code Smells, that can be identified:

• TimerManager (a weak smell) - Everything that is called a manager alerts me a great deal.
  Usually managers are God objects, has low cohesion and high coupling and violating too much
  OO principles...
  It doesn't necessary mean that the same happens here, but it should be noted and then verified in the code.
• TimerCallback should be generic or restricted by a Data Type.
  It's better to specialize the TimerCallback with more concrete type parameter. Having said
  that, it might be that the author wanted to use the built in TimerCallback in .NET.
  In such a case the declaration is redundant.
• static _sync object.
  The class has only instance members, besides that _sync object; Will it make sense to do the following?

TimerManager timerManager1 = new TimerManager();
timerManager1.SetTimeout(...);
... 
TimerManager timerManager2 = new TimerManager();
// here timerManager2 will block on the same object as the timerManager1
timerManager2.SetTimeout(...);

• Both Dictionaries are just plain procedural programming definitions; It is the same like having 2 arrays aligned by their indexes as both the dictionaries will be accessed by the same index (an int). Behold, as one mistake (accessing the wrong index) can lead this code to an undefined behavior.
• GetHashCode shouldn't be used to identify uniquely an object.
  Let me repeat again... GetHashCode mustn't be used to identify uniquely an object...
  GetHashCode's implementation is needed for collections (and Equals, see below).
  Sometimes the same result of the hash code will put two different object in the same hush bucket - which means those are not unique identifiers!!!
• Once implementing GetHashCode, the Equals method should be implemented as well.
• Consider the follwoing code:

t.Stop();
t.Close();

• One should be familiar with the tools at the hand.
  Close() does what Stop() do and more... Calling Stop() and then Close() just makes the method a little bit longer without any effect.
  When in doubt, consider to take a few moments just to review the provided API.
• Redundant Remark.
  The remark repeats the code, doesn't bring anything informative enough and affects the length of the method.
  It can be safely removed.

// invoke function provided by caller
timerCallback(null);