Wednesday, December 14, 2011

Do one thing.

Classes should do one thing.

If they have "other stuff" to do, then they should use other classes.

Embedding the implementation of thread-safe, dictionaries of linked lists in your functions fails on several levels

  • Next time you need a "Multimap" you will write it all over again.
  • There's a name for this data type, that should be a hint not to write it yourself.
  • Your function is now implementing what it's supposed to, and it's implementing MultiMap.Add()
  • You have lock primitives in open code.
  • Your code is harder to read. You now have many lines of code in your function. These lines could be replaced by Multimap.Add(). That would simplify your code and make it easier to see the main functionality.
  • If just above the section of code you have a comment saying "add this to a thread safe multimap" then shame on you, you already knew it should not be there.
  • If you have enclosed it in a #region (c#) and collapsed that region to a single line comment, then was this not a big hint that it could be taken out and put in it's own class.
  • Your code is now much harder to test. How do you test the MultiMap code which is embedded in your function.
  • If you need to update the Dictionary of Lists for performance, it's going to be fun fixing it in many different places.
  • And sooner or later someone will read or update your dictionaryOfLists without noticing the dictionaryOfListsLockObject.
I do appreciate that a dictionary of lists is simple, and that you have probably implemented it correctly. In fact you have implemented it so many times by now, you can do it in your sleep. 

Please appreciate that this is not a good thing to boast about.

For further reading google "principle of single responsibility" or look here

No comments:

Post a Comment