The second case looks like a classic "On Error Resume Next" style of code that is trying to be helpful but I think is generally far more harmful. It can be hard to explain to less-experienced developers why hard failure is preferable to trying to helpfully handle error cases , at least until they learn the pain of debugging a system that isn't giving you a clear signal of an error and instead just does the wrong thing.

The first case is interesting to me: I often overlook the cost of virtual dispatch for the sake of making the contracts between components clearer. I wonder if a happier middle-ground would be to use ImmutableDictionary? I much prefer the strong behaviour (you can never change this dictionary) versus the weaker contract (you can't modify this dictionary, but we don't promise nobody else will), and you'd eliminate the virtual dispatch too.

In the first case, if the code is used within a single (trusted) assembly, shouldn't the interface be internal instead of public ? That way, you can safely expose a Dictionary and know that it (probably) won't be modified.

GetLastProcessedDocumentTombstonesPerCollection ... well if you're going to add documentation in method names I suggest to make it more detailed ... something like 'GetLastProcessedDocumentTombstonesPerCollectionSinceThisMethodWasCalledAndTheDocumentsWereMakedAToucheOrTheLastSinceStartOf ... TheEnd' ;)

Joking aside I think a shorter name like GetLatestThombsones with doc comments are actually more readable.

@Ayende, I think well-placed doc comments in internal code can greatly reduce source code churn for developer onboarding. Source code churn can be a steep artificial barrier to entry for new developers. You don't need to read the code and know how a settings cache is implemented (until you actually have a need to do it), a description is enough if captures the purpose. IE: "stores the settings in memory and automatically reloads them whenever an update operation is intercepted from ...." This can save minutes to hours of code reading in a phase the developer should skip it anyway.

It looks like I was part of that conversation on the BCL immutable collections four years ago. I had forgotten how badly they under-perform for your use cases. I see that you ended up with your own ImmutableAppendOnlyList for Voron. I don't suppose you have another magnificent type that fits this case?

There is a reason why comments rot is an insidious thing

It's the same reason source code rots, people who don't take that extra 5 min at the end of a change to refactor and clean their changes.

I would rather have clear code

Given the choice me too. If however, I can have both, clear code and doc comments, I would gladly take both.

IMHO, in the first case, it is always better to have more “readonly” and “immutable” stuff, always, always. Even for internal works. And change it only if you have to. It’s a pity that containers you mention has performance issues, but if you do not implement super-highly-performant services that these concerns look like optimizing too early.

For the second case I prefer to do more checking. Is it contrary to fail early? Probably. But if you have many components interacting with each other IMHO it is better to they be more forgiving to each other then expecting very strictly.

Having explicit READONLY is great, but definitely not at the cost of performance.

In general, in C# any collection interface is a code smell (except IEnumerable of course). People dragging their Java/C++ wisdom into C# create undue trouble, so keep bashing them on small points and they'll learn quicker.

If CLR did have a decent ReadonlyDictionary, that's what would be sensible to use. Of course, ImmutableDictionary isn't a trivial replacement perf-wise, especially in edge cases. Whether it fits here — hard to say. It may well fit, memory-wise it could win over flatter sparser hashtable implementation.

Of course, if you don't use ImmutableDictionary widely anyway, picking it for one specific use case hurts maintainability. Every time developer stumbles upon it, she has to STOP AND THINK. Wow, is this important? Why not normal? Thinking leads to errors.

Checking for null should be done using if statements. Apply ruler on the fingers of those who disobeys.

Note how ?? needs a comment? That's why. A dumb if (existingData==null) wouldn't, BECAUSE IT'S EASIER TO UNDERSTAND WITHOUT COMMENTS.

If existingData cannot be null, fine, remove it. Otherwise ask them to share the single empty instance, save on memory. Obvious plus for performance-sensitive code!

Oren, so you are right if performance is top priority. Respect! Oleg, how do you know it!? Respect to you too! :) I was C++ developer for years. After switching to C# I’m constantly miss two things. “const” for the first case which highly promotes and spread immutability in the system. References for the second case when you do not have to check for nulls. BTW. C++ is associated with pointer but in C# are all pointers everywhere because you have to check for nulls.

End in end maybe some functional languages are rescue here? (both for immutability and performance)

Oren, not saying ?? is a bad thing — but not for this use case (hence comments). Having an explicit if here would be better.

Not least because semantically it's different: assignment will only happen in edge case. The original example has existingData redundantly assigned to itself in the much-used hot path.

Andrzej, the non-nullability is being added to C# in the next iteration.

Frankly, I don't like it: the checks are being bolted on the side, with no guarantees and whole infrastructure reliant on good intentions rather than fact. It may have been better integrated in C++, although the mental cost of unpicking const-related compiler errors/warnings is a burden.