 |
|
|
After pondering your code for a while longer I came to the conclusion that all you're really doing is wrapping the
IMyObject item = GetMyObjectFromDB(key);
to enable the caching of its results. A worthy goal in many instances, as stated in your article. So I set out to find a more elegant way than that which your article presents. Once I saw the crux of the problem I realized that a delegate would be an excellent solution.
The main part of the solution is a class to hold the cache, to instantiate it you pass in the required delegate. (The interface isn't required, but it could come in handy at some point.)
public interface IGenericCache<TKey,TValue> { TValue GetItem ( TKey Key ) ; } public class GenericCache<TKey,TValue> : IGenericCache<TKey,TValue> { public delegate TValue GetItemDelegate ( TKey Key ) ; private readonly GetItemDelegate itemgetter ; private readonly System.Collections.Generic.Dictionary<TKey,TValue> cache ; public GenericCache ( GetItemDelegate GetItem ) { if ( GetItem == null ) { throw ( new System.ArgumentNullException ( "GetItem" , "You must provide a delegate" ) ) ; } this.itemgetter = GetItem ; this.cache = new System.Collections.Generic.Dictionary<TKey,TValue>() ; return ; } public virtual TValue GetItem ( TKey Key ) { lock ( this.cache ) { if ( !this.cache.ContainsKey ( Key ) ) { this.cache [ Key ] = this.itemgetter ( Key ) ; } return ( this.cache [ Key ] ) ; } } }
This class performs all the required functionality. I added the lock for a little thread safety, I'm not sure how effective it'll be. And as I write this I realized that maybe I should have made it IDisposible.
Usage:
GenericCache<string,int> g = new GenericCache<string,int> ( int.Parse ) ; System.Console.WriteLine ( g.GetItem ( args [ 0 ] ) ) ; System.Console.WriteLine ( g.GetItem ( args [ 0 ] ) ) ;
Or you can derive a specific cache:
public sealed class MyCache : GenericCache<string,int> { public MyCache ( ) : base ( int.Parse ) { return ; } }
You could enforce this type of derivation by making GenericCache abstract.
Usage:
MyCache m = new MyCache() ; System.Console.WriteLine ( m.GetItem ( args [ 0 ] ) ) ; System.Console.WriteLine ( m.GetItem ( args [ 0 ] ) ) ;
But that doesn't give you the singleton-ness you want, so you could also wrap one in a static class:
public static class YourCache { private static readonly GenericCache<string,int> cache = new GenericCache<string,int> ( int.Parse ) ; public static int GetItem ( string Key ) { return ( cache.GetItem ( Key ) ) ; } }
Usage:
System.Console.WriteLine ( YourCache.GetItem ( args [ 0 ] ) ) ; System.Console.WriteLine ( YourCache.GetItem ( args [ 0 ] ) ) ;
Well? Whaddaya think?
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
Very cool! I'm not sure it's actually much simpler in use, but I'm certainly beginning to see your singleton arguments now and your implementation does look 'cleaner'.
I really appreciate you taking the time for this. Thanks!
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
Glad to be of service.
Another thing I forgot to mention about your implementation is that it's generally considered poor design for a base class to require knowledge of a derived class.
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
 |
|
|
 |
|
|
 |
|
|
Yes, as the profile says, I just think that llamas could probably be raised anywhere. Wouldn't mind a few myself.
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
This be made more efficient by using TryGetValue outside the lock and only locking if adding to the cache.
More here:
http://www.ayende.com/Blog/archive/2008/01/21/Performance-threading-and-double-checked-locks.aspx
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
I considered that, but I'm not real concerned about performance there.
P.S. And it looks like Jon Skeet agrees with me, so I'll keep doing it this way.
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
I'm afraid it's not possible. Because reading and writing in a dictionnary is not threadsafe. reading only is threadsafe, but not with writing.
I also tried using readerwriter lock, but upgrading from read to write cannot be used as it is possible to have two reader that want to upgrade to write at the same time.
Maybe the new ReaderWriterSlimLock from .net 3.5 can solve this issue, but I didn't tested it yet.
My advise when doing multithreading is : read carefully the MSDN documentation, and think a lot before writing anything. It can save a lot of time... because these errors are very difficult to find.
Regards
|
| Sign In·View Thread·PermaLink | 5.00/5 (1 vote) |
|
|
|
 |
|
|
If you only want one instance of a class then only create one instance of that class. Why is a singleton necessary?
The tenets of object oriented programming are encapsulation, inheritance, and polymorphism. How many of these do singletons violate?
Why isn't your class abstract? That would be much better than throwing an exception if a method isn't overriden. It's not like you can use it on its own anyway.
How would you suggest mocking this class for testability?
Your use of reflection to create an instance of the class is also completely unnecessary:
public static readonly T Instance = new T(); (you'd have to adjust your generic constraints for this to work).
There are so many things wrong with this design... I recommend you reconsider your approach.
|
| Sign In·View Thread·PermaLink | 5.00/5 (1 vote) |
|
|
|
 |
|
|
Hi jonnii,
Blimey - that was pretty scathing!
OK, firstly the class is now abstract - I've asked codeproject to update it but they haven't yet. It was my original intention but it got lost somewhere in my attempts to get it working.
The reason I use reflection to create the class is because of the singleton pattern. If I don't use reflection then the derived class has to have a public constructor so that I can instantiate it. That's not what I want.
I'm interested in your general argument regarding singletons (PIEBaldConsult seems to agree with you on this so hopefully he/she will chip in), can you explain what's so bad about singletons and what would you do instead? I'd kinda been under the impression it was a pretty standard pattern (GoF) and widely used.
Regarding testing - I've already tested it (not that there's much to test) - no mocking involved.
I'm humble enough to have my mind changed on this, but at the moment I'm finding the pattern very helpful and neat. If you have a better way of doing this, could you please share it with me so I can consider changing my code.
Thanks, LJ
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
 |
|
|
I apologise if my comment came across as scathing, that was far from my original intention. I guess I'm just blunt!
Many articles have been written about why the singleton is an antipattern, if you google for 'singleton anti pattern', you'll get a good selection. My favourite one is probably this one:
http://googletesting.blogspot.com/2008/08/root-cause-of-singletons.html
Personally I wouldn't use a singleton for the same reason I wouldn't use a global variable. You have all kinds of problems for example access control, threading issues, etc... and when testing you need to "reset" the singleton object into a known state before each test because they can't be mocked out and isolated using a mocking library like RhinoMocks.
As far as I'm concerned every class should have only one responsibility. Your caching class should be responsible for caching, not for mainintaining its own lifecycle. By doing this you make it more likely that your class will be reused, especially because you should never make assumptions about how others will use your code.
I use a dependency injection container (specifically Windsor), and parameterise my classes from above. So in this case I would most likely have an ISomethingCache which would be injected into my ISomethingBusinessObject.
In regards to the GoF patterns. I think they are a great thing because they give all programmers a common lexicon from which to describe their code, but at the same time many of the patterns are designed to act as work arounds in languages that aren't expressive enough.
For example, would use you use the Observer pattern in C#? Do you need the singleton pattern when you have static methods? Would you use the iterator pattern when you have IEnumerable and foreach?
I hope this post helps! Let me know if you want me to elaborate on anything.
|
| Sign In·View Thread·PermaLink | 5.00/5 (1 vote) |
|
|
|
 |
|
|
Excellent.
jonnii wrote: you should never make assumptions about how others will use your code
Exactly.
jonnii wrote: Do you need the singleton pattern when you have static methods?
And static classes.
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
"you should never make assumptions about how others will use your code"
I agree, but in this case the code is internal so only I'm using it and I know what it does and I don't have to worry about anyone else.
"Do you need the singleton pattern when you have static methods?" "And static classes."
Can't argue with that (well I can't think of an argument yet anyway )
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
I would suggest that a surprisingly large percent of the time you think you'll only ever need one of something, you'll eventually find that you're mistaken.
I'm relatively new to .net, but I would think that threadstatic variables would be the way to go. To minimize memory use on threads that don't need certain variables, one should put all the variables one will need into a small number (possibly one) of classes, and have one threadstatic variable for each class. Any thread which needs the variables in a class can create its own instance of that class.
If threadstatic variables are used and a means exists to save/restore state, then one can avoid having to pass oodles of parameters into nested procedures. If any procedure that will change the threadstatic state for its own purposes saves the state on entry (or before changing it) and restores it on exit (or after the last change), things should work fine.
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
Hey Jonnii no offence taken, but it was certainly a robust reply After the initial jaw-drop I'm actually finding this a very positive experience - it's nice to get some educated opinion (I include the other posters in that also).
I see your point about the global variable comparison. Can you explain the dependency injection idea a bit further. I'm a bit confused how injecting a cache into a business object would work? Assuming a class Company that contains a list of items of class People would I inject the cache into the Company (parent) object and then ask the company object to return me a person (which would then be retrieved from the ISomethingCache if it could be)? Is there anyway you could knock up an example or point me at one (sorry if that's asking a bit much).
I agree entirely with your points about the GoF patterns and patterns in general. It's actually very easy to fall into a particular way of doing things - and it's good to be challenged now and again. That said, I feel that it's equally easy to follow the herd in the latest ideas when it's not always appropriate or understood. For example I think saying that "all singletons are evil" is as bad as saying "all singletons are good" (that's not aimed at you by the way, I was just using singleton as an example). It often comes down to circumstances.
Thanks to everyone for the interesting posts.
LJ
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
I recommend you do some reading into DI. There are a few popular containers out there, for example Ninject and Castle Windsor (this is the one I use), and their respective websites contain lots of useful examples.
In the example you stated, your Company and People objects would be domain objects, not business objects. You'd probably have something like:
interface ICompanyBusinessObject{ Company Find(CompanyName companyName); }
which might do something like:
public Company Find(CompanyName companyName){ Company company = companyCache.Find(companyName); if(company == null){ company = companyRepository.LoadCompany(companyName); company.People = peopleBusinessObject.FindAllFor(company); companyCache.Add(company); } return company; }
This is a really bad example, but it's the best I could come up with off the top of my head.
For the record, I'm not saying singletons are bad, I'm just saying they are frequently incorrectly applied. I'm guilty of doing stuff like:
private static readonly ILog log = LogManager.GetLogger();
Which is akin to using a singleton. I justify it in that circumstance because I don't want to have to inject all my logger instances.
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
Wow. Good article, thanks, will read the related links in a bit...
::later::
Just finished. The one from IBM says:
" A classic example of a true singleton is a logging service. "
Funny that my logging service isn't a singleton, I allow my code (Windows Services) to use either a common log or specify an individual one.
Oh, here's[^] a link to a thread I started when I was doing similar. I eventually simplified the whole mess, as I mentioned.
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
 |
|
|
Should LazyLoadCacheBase and GetItem be abstract?
Insisting that the class have a private constructor and then not abiding by that restriction seems very poor form. If you can violate it, anyone can violate it.
I'm reminded of something I wrote a few months ago that seemed too complex; eventually I ended up with a much simpler and more flexible solution by eliminating certain limitations I had imposed. In your case I suggest removing the limitation that it only work with singletons.
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
PIEBALDconsult,
"Insisting that the class have a private constructor and then not abiding by that restriction seems very poor form. If you can violate it, anyone can violate it." I'm not sure this comment makes much sense as anyone can violate any class with a private constructor using reflection and mine is no different.
You are absolutely correct about making it abstract - I had originally intended to, but having had to get around a few problems making it work I'd changed it at some point and not changed it back again (I was typing this in very early in the morning). I will request an update to the article to correct that.
The singleton pattern was exactly what I wanted and it was the combination of that along with the generics and the template and lazy load pattern that I found difficult to achieve. Just figured it might help someone out there.
Thanks for your comments. LJ.
modified on Thursday, September 4, 2008 5:00 AM
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
Laughing.John wrote: anyone can violate any class
Exactly. So if you're just going to bypass the restriction anyway, why require it? Why not allow "normal" classes?
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |
|
|
Well I suppose you could think of it like a burglar alarm - it's designed to put the burglar off, but it won't actually prevent you being robbed ...
You are effectively saying that reflection has made the singleton useless. Whilst that's kind of true I think a singleton at least encourages the intended use pattern.
In my case I only want one cache of each object type and the pattern encourages that.
|
| Sign In·View Thread·PermaLink | |
|
|
|
 |