Click here to Skip to main content
Click here to Skip to main content
Technical Blog

Tagged as

Code Smells

, 4 Jan 2011 CDDL
Rate this:
Please Sign up or sign in to vote.
(Cross post from IRefactor)If you like practicing in identifying code smells, then you can find below a short class calledTimerManager.public class TimerManager{ public delegate void TimerCallback(object data); private static readonly object _sync = new object(); private readonly Dictionary
(Cross post from IRefactor)

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);

License

This article, along with any associated source code and files, is licensed under The Common Development and Distribution License (CDDL)

Share

About the Author

Uri Lavi
Other
Israel Israel
Uri Lavi is a development lead with extensive experience in Data Intensive, Business Compound, Distributed and Scalable Software Systems. Uri specializes in mentoring, coaching and consulting for complex software engineering topics, among which: Software Architecture, Design Patterns & Refactoring.

Comments and Discussions

 
-- There are no messages in this forum --
| Advertise | Privacy | Mobile
Web01 | 2.8.141015.1 | Last Updated 4 Jan 2011
Article Copyright 2011 by Uri Lavi
Everything else Copyright © CodeProject, 1999-2014
Terms of Service
Layout: fixed | fluid