views:

153

answers:

5

I have a Dictionary<string,int> and I simply want to decrement the value in my dictionary by one.

I have this but not sure if its best practice.

foreach (KeyValuePair<string, int> i in EPCs)
{
   EPCs[i.Key] = i.Value - 1;
}

UPDATE: The reason I am trying to decrement the value is becase the value is a index number relating to a position. When I remove something from the dictionary I then have to decrement that index number in the dictionary. There may be a better way.

+2  A: 

I think this is completely appropriate.

But since you asked the question, what are you concerned about that may not be reasonable about this kind of code?

You should realize that you have two options to do what you're looking for, either:

  1. Modify the existing dictionary by visiting each entry (which your code does), or
  2. Create a new dictionary with the computed values you want.

You can do the second easily with LINQ:

var newDict = myDict.ToDictionary( kvp => kvp.Key, kvp => kvp.Value-1 );
LBushkin
I'm curious why this was the accepted answer when it contains the same information as my answer, which is older by a few minutes and higher voted?
Adam Robinson
I have removed it as when I ran the code I got an exception about the collection being modified therefore I guess my code is not correct as both answers indicate.
Jon
A: 

You can write a little for-each enumerator yourself that takes an action and executes it on every element:

    public static void ForEach<T>(this IEnumerable<T> source, Action<T> action)
    {
        foreach (T element in source)
        {
            action(element);
        }
    }

Use it like this:

 EPCs.ForEach(x => EPCs[x.Key] = x.Value -1);

It's not exactly any cleaner than what you had before but a little more compact though. The Reactive Extensions have a similar operator in System.Interactive called Do.

Johannes Rudolph
`KeyValuePair` is a read-only type. You cannot modify the contents in this way.
Adam Robinson
That won't work. The `Value` property of `KeyValuePair` does not have a setter - you cannot assign to dictionaries in this way.
LBushkin
Yes, fixed it. I didn't exactly copy-paste his code and thus missed it ;-)
Johannes Rudolph
I've never understood the desire to use the `ForEach` function on `List<T>`, and I think there's a reason it was not included in the `Enumerable` class. How is this that much less verbose than a standard `foreach` loop like he has? Simply because you're eliminating the words `var` and `in`? This is otherwise identical, other than the fact that you have to use a lambda rather than a normal (faster) sequental set of instructions...?
Adam Robinson
It *is* identical and no, it's not significantly slower. The lambda will compile into an anonymous function that is called for every element. Having a `ForEach` extension is useful when composing enumerable operators for introducing side effects. The OP tagged his question linq and I therefore think he's searching for a linq-style solution. The suggested `ForEach` operator statisfies this.
Johannes Rudolph
@Johannes: Not significantly, but why introduce *any* performance penalty when there's no benefit? The lambda will compile into an anonymous function, then a static delegate instaince. Upon the function call, that delegate will be null-checked, then assigned if null. The delegate value will then be passed into `ForEach`. No, not terribly expensive, but what's the point when it gains you -- literally -- nothing.
Adam Robinson
@Adam: It gains me composable data-pipelines with side effects, a more compact way to write a foreach loop and a linq-style operator that does the job the OP want's it to do. The same argument you brought forward can be made for `.Sum()`, why would anyone need it, it's trivial to implement ?! It's cleaner and easier to read. The level of abstraction is higher. Sum/ForEach could use parallel computation *transparent* to the caller. Have you ever looked into map-reduce?
Johannes Rudolph
My argument is that the `ForEach` function is no simpler; you're simply eliminating two words from the statement, nothing more. The same cannot be said for `Sum`. Yes, it's trivial, but you don't have to declare and maintain an aggregation variable. You *get* something. With `ForEach` (in this circumstance) you quite literally get nothing except background code that's entirely needless. I'll certainly concede that there are corner cases where it *can* be useful, but this does not appear to be one of them.
Adam Robinson
+5  A: 

Your existing code is an entirely appropriate way to decrement all of the values in a dictionary.

If you wanted to create a new dictionary, you could use LINQ:

EPCs = EPCs.ToDictionary(p => p.Key, p => p.Value - 1);

This will, however, create an entirely new Dictionary<string, int> instance, rather than modifying the existing instance in place. However, since you tagged your question with linq, I figured I would offer the one way (that I'm aware of) where LINQ could solve your problem.

Adam Robinson
LINQ operations work on immutable types, so you're correct that there is no way to do it with LINQ without creating a new dictionary.
BlueRaja - Danny Pflughoeft
@BlueRaja: I think it's more correct to say that LINQ operations are not *designed to modify existing enumerables*. There's nothing in LINQ that makes it work on immutable types any differently than it does on mutable types.
Adam Robinson
A: 

Your code is perfectly fine given the circumstances (Dictionary<string,int>).

If you need high performance using something else than a dictionary might be a better choice in the long run.

Foxfire
+2  A: 

This is not a direct answer, but instead of decrementing it for each item you could just store an offset and decrement it on the fly when getting an item, either as specialized class or just in the code in general.

Lucero
And the runtime complexity award goes to... ;-)
efi