views:

437

answers:

3

Hello:

A TimeSheetActivity class has a collection of Allocations. An Allocation is a value object that is used by other objects in the domain as well, looking something like this:

public class Allocation : ValueObject
{
    public virtual StaffMember StaffMember { get; private set; }
    public virtual TimeSheetActivity Activity { get; private set; }
    public virtual DateTime EventDate { get ... }
    public virtual TimeQuantity TimeSpent { get ... }
}

Duplicate allocations for the same Allocation.EventDate are not allowed. So when a client attempts to make an allocation to an activity, a check is made to see if there is already an Allocation in the collection for the same Allocation.EventDate. If not, then the new Allocation is added to the collection, but if so, the existing Allocation is replaced by the new one.

I am currently using a Dictionary to maintain the collection, with the Allocation.EventDate as the key. It works fine for the domain but I'm wondering if the fact that the key is already part of the value isn't itself a 'bad smell'.

I also have no reason to persist anything but the dictionary values. Because I'm using NHibernate, I may need to write some custom type do that, and I'm wondering if that also is a clue that I should be using a different type of collection. (That's also the reason for the the virtual properties in the Allocation class).

The primary alternative I'm thinking of would be a HashSet with a dedicated EqualityComparer.

What do you think?

Cheers, Berryl

+2  A: 

I don't think the fact that the key is part of the value is necessarily a problem. In my experience that's quite frequently the case for dictionaries. A HashSet with an appropriate equality comparer would certainly work, so long as you never needed to get at the current object with a particular EventDate. That seems like a useful thing to be able to do, potentially...

Are you currently just concerned in a somewhat vague way, or do you have a concrete suspicion as to how this might bite you?

Jon Skeet
I really wanted to validate the somewhat vague concern about the key being part of the value. Truth be told the dictionary works great and is easy to use.I just finished Part 1 of your book, btw ... you're a wonderful writer!
Berryl
+3  A: 

If you use an external comparer to do a HashSet<Allocation>, you would have to be very careful not to change the date without re-keying the value in the dictionary. You have this problem either way, but it is exacerbated by mutability (at least with Dictionary<,> it will still be able to track its own keys and values). With a mutable value as part of the key you risk never seeing the value again...

When I've worked on schedule systems in the past I've actually used SortedList<,> for this - similar, but useful if you generally want the data in sequence, and allows binary search if the data is fairly uniform.

Marc Gravell
Thanks for the tip on SortedList. Cheers
Berryl
+1  A: 

Using the standard libraries, there is no better solution that I know of. However, I also felt this kind of code to be a "bad smell", but it's because of the collection classes in the BCL and not your code.

I don't know why MS didn't create generic Sets<TKey, TData> where TData: IKeyed<TKey> or so instead of Dictionaries, because this would allow implementing such data structures where the key is part of the data. The KeyValuePair<TKey, TValue>: IKeyed<TKey> would just be a helper structure implementing this interface and thus allowing to create identical dictionary functionality as we have it now.

Also (to continue with the small rant) I wonder why they did not add the notion of declarative immutable types and make this a possible generic type constraint, because this would allow to make sure a key does not change its hashcode during runtime (which can happen currently if one implements GetHashCode() and Equals() on some mutable object).

Lucero