Command verb "list" for developer command /telepoi #1066
Conversation
…hes and lists all POIs
Source/ACE.Database/WorldDatabase.cs
Outdated
| /// </summary> | ||
| public void CacheAllPointsOfInterest() | ||
| /// <returns>time used</returns> | ||
| public TimeSpan CacheAllPointsOfInterest() |
There was a problem hiding this comment.
I'd prefer this function not return the duration it took to do the work.
If a consumer of the function is interested in that information, it should calculate itself.
| /// <summary> | ||
| /// Returns the PointsOfInterest cache. | ||
| /// </summary> | ||
| public ConcurrentDictionary<string, PointsOfInterest> GetPointsOfInterestCache() |
There was a problem hiding this comment.
We shouldn't expose the base cache object.
Instead, if you want the elements, you should create a copy. We want to make sure a consumer doesn't have direct access to our base cache objects. They could end up doing something we don't want.
There was a problem hiding this comment.
I debated this with myself, and another developer and we agreed that it should probably return the meat of it instead of only a certain property in case someone wants to do something more like show the coordinates in the list as well. I started out having it return IEnumerable<string> and switched to ConcurrentDictionary<string, PointsOfInterest> after thinking about it further. I understand the need to prevent data from being tainted but really IMHO none of the consumers should be doing that, and if they are it's a bug. The alternative is to write accessors for every specific type of application within that layer which I'm sure you would agree is crap, or like you said copy the objects, which is almost as crappy, now we have to go through and make deep copy procedures for every applicable class involved for it. If it were up to me I would not copy it and just remember not to try and do any write operations to it from other layers. I'll make this change, but I disagree with that "just copy everything" platitude which I myself sometimes fall into because of sheer exhaustion because I'm just too tired of keeping track of what can be modified and what can't.
There was a problem hiding this comment.
We don't need to deep copy the elements, we just don't want to return access to the base cache dictionary.
All of the various objects returned by the various world database caching mechanisms are the same, not clones.
There was a problem hiding this comment.
I think a simple line like this will suffice:
return new Dictionary<string, PointsOfInterest>(cachedPointsOfInterest);
There was a problem hiding this comment.
I didn't know that, should have asked. Thanks, I'll handle it. won't the individual elements in the list be vulnerable still?
There was a problem hiding this comment.
The individual elements will be shared by anyone using the cache, and yes, vulnerable to mutation.
This is how all of our world db cache works.
If you want to mutate those objects, you should then clone them. This would be the responsibility of the consumer, not the cache (as it is currently designed).
For weenies, we clone them into biotas and then mutate the biotas.
removing return value of POI caching function
(re)caches and lists all POIs