Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 6,124 | Comments: 45,475

filter by tags archive

Comments

Ryan Schipper

A couple of things that need consideration in my opinion.

1) in my opinion it is inappropriate to force your API consumer to use exception handlers for normal process flow as demonstrated in the first few tests

2) a dictionary is a reasonably established paradigm and standard practice (at least where I come from) would suggest that the add function should be add(key, value). Instead, you have an explicitly required container class.

One of these may even be what you are thinking of..

Anon

Issue is very simple. I am annoyed with you because you have not posted anything for 9 days. I hope all is well, but I'm annoyed anyway.

Kristof Claes

Did it have something to do with the fact that before you add an item, you have to check if it already exists or not and trying to get an unregistered item throws an exeption?

Thomas Eyde

You found out that you wanted a set, not a dictionary?

More specifically, deleting a non existing item can be considered already deleted, adding an existing item can be considered as an overwrite, and retrieving a non existing item could return null.

I am not sure about the last one, but add and delete in this fashion should not throw.

fschwiet

Just yesterday I hit a bug where within one session I read an item (discarding the result) then wrote an item with the same key. This caused an exception, as would be expected by the last test. I would've preferred it would just overwrite. I assumed I was doing it wrong, maybe that will change.

The design problem may be that adding an item may throw an exception (if it has same key as existing), while the only way the caller can check if that will happen (by loading whatever has that key) could also throw an exception. So any typical use then involves handling an exception.

Just feedback on the last test... Its better to only have the one line that will throw an exception within the Assert.Throws() expression. Otherwise this test may pass for the wrong reason when an earlier line fails inappropriately.

Rik Hemsley

Going by what I see in the tests, ApplicationInstanceDictionary is no different from Dictionary <guid,> , so you should use the .NET class and this means you don't need to test it.

Kevin McCormick

Since this collection is tied to the class, the guid/key restrictions are kind of unnecessary. The dictionary class should manage duplicates, keys, etc. internally, and you should be able to manipulate the collection via index, linq, and provide indexing by guid only as an added bonus (ie, myColl.FindByGuid() ) if it's really necessary.

Rik Hemsley

When I said Dictionary, the blog software ate my angle brackets, because it's rubbish (it still fails to 'remember me' despite offering to). So I meant a dictionary of Guid => string.

Steve Py

If ApplicationInstanceDictionary extends Dictionary, then it would appear that you are testing the default behaviour of a Dictionary, though the types of exceptions thrown might be different.

Andrey Titov

You changed them to TryRemove, TryAdd and TryGet to support concurrency?

Martin O'Keefe

'ApplicationInstance'

You need to maintain references to the GUID keys - the keys are required by consumers of values.

I would have suggested making the key the Type, but you are passing two strings as values - that's out.

I'm assuming that you chose GUID because the values are not user configurable - otherwise const string keys would appear to be better.

If the key is to be dynamic it seems that you need to make the key deterministic based on the type and its scope etc.

Frank Quednau

Just from the API perspective I don't like the asymmetry of adding ApplicationInstances and removing Guids.

Jon Norton

In CannotAddDuplicatedItems() the test could pass due to an InvalidOperationException being thown before the last Add where the test is actually expecting the exception to be thrown. The Arrange portion of the test is included in the Action and Assert section represented by the Assert.Throws.

meo
meo

TMM, it is strange design, when you have 'firstInstance' != 'secondInstance' due to same key and different values in it (in 3rd test), but adding second after the first one will fail with InvalidOperation. It's inconsistent

meisinger

the exception type is wrong

either that or you have determined that writing unit tests are silly and not needed

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. The design of RavenDB 4.0: Making Lucene reliable - 2 hours from now
  2. RavenDB 3.5 whirl wind tour: I’ll find who is taking my I/O bandwidth and they SHALL pay - about one day from now
  3. The design of RavenDB 4.0: Physically segregating collections - 2 days from now
  4. RavenDB 3.5 Whirlwind tour: I need to be free to explore my data - 3 days from now
  5. RavenDB 3.5 whirl wind tour: I'll have the 3+1 goodies to go, please - 6 days from now

And 13 more posts are pending...

There are posts all the way to May 30, 2016

RECENT SERIES

  1. RavenDB 3.5 whirl wind tour (14):
    02 May 2016 - You want all the data, you can’t handle all the data
  2. The design of RavenDB 4.0 (13):
    28 Apr 2016 - The implications of the blittable format
  3. Tasks for the new comer (2):
    15 Apr 2016 - Quartz.NET with RavenDB
  4. Code through the looking glass (5):
    18 Mar 2016 - And a linear search to rule them
  5. Find the bug (8):
    29 Feb 2016 - When you can't rely on your own identity
View all series

RECENT COMMENTS

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats