Monday, January 3, 2011

Code Smells

(Cross post from IRefactor)

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

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);
               x =>
                  lock (_sync)
                     var t = timers[x.GetHashCode()];
                  // invoke function provided by caller
            timer.Elapsed += timer_Elapsed;
            timer.AutoReset = false;

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, have 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();
TimerManager timerManager2 = new TimerManager();
// here timerManager2 will block on the same object as the timerManager1
  • 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 objects 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:
  • 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
  • Thanks to Aviv the following is of course another smell: passing null to the callback function (i.e. declaring the parameter in the callback and eventually passing null).

No comments:

Post a Comment