Proper use of CodeContract Requires

Question

Basically I want to know if using a code contract to determine if a key exists in a ConcurrentDictionary is an acceptable use of a code contract. It doesn't feel right to me because it's more than parameter check as it depends on the state of the dictionary at runtime.

public class MyClass
{
    private ConcurrentDictionary<string, object> someItems = 
        new ConcurrentDictionary<string, object>();

    public object GetItem(string itemName)
    {
        Contract.Requires<ArgumentNullException>(!String.IsNullOrWhiteSpace(itemName));

        // ?? Is this a correct alternative to checking for null???
        Contract.Requires<KeyNotFoundException>(someItems.ContainsKey(itemName));

        return someItems[itemName];
    }
}

But if it is okay, it's a cleaner method that has 2 Contract.Requires and one return, over the traditional way below.

public class MyClass
{
    private ConcurrentDictionary<string, object> someItems = 
        new ConcurrentDictionary<string, object>();

    public object GetItem(string itemName)
    {
        Contract.Requires<ArgumentNullException>(!String.IsNullOrWhiteSpace(itemName));

        // Traditional null check
        var item = someItems[itemName];

        if (item == null)
        {
            throw new KeyNotFoundException("Item " + itemName + " not found.");
        }

        return item;            
    }
}

Alexander Tsvetkov · Accepted Answer · 2013-03-15T22:31:43.943

0

Looks a bit odd to me: the internal state of the class (i.e. the contents of someItems dictionary) is definitely not a part of a contract. You can use it this way, but contract checks are supposed to let the caller know what exactly was wrong. In this case a caller will never guess which arguments are allowed and which are not - even after getting KeyNotFoundException.

Moreover, in this particular case throwing an exception if object is not found in dictionary does not look consistent, in my opinion it would make more sense to return null.

By the way, in your first example itemName is searched twice in the dictionary.

edited Mar 15 '13 at 22:31

answered Mar 15 '13 at 22:20

Alexander Tsvetkov

1,649
14
24

KeyNotFoundException on a method GetItem with one argument pretty much tells me what exactly was wrong; however, I tend to have the same feeling about this particular test not being a contract per se. Throwing vs. returning null is a design consideration for the API, not really a code contract concern. Overall I think you've validated my same concern with nice points. Thanks! – GoClimbColorado Mar 16 '13 at 18:00

Proper use of CodeContract Requires

1 Answers1