2

I've read on here that iterating though a dictionary is generally considered abusing the data structure and to use something else.

However, I'm having trouble coming up with a better way to accomplish what I'm trying to do.

When a tag is scanned I use its ID as the key and the value is a list of zones it was seen in. About every second I check to see if a tag in my dictionary has been seen in two or more zones and if it has, queue it up for some calculations. 

for (int i = 0; i < TagReads.Count; i++)
{
    var tag = TagReads.ElementAt(i).Value;
    if (tag.ZoneReads.Count > 1)
    {
        Report.Tags.Enqueue(tag);
        Boolean success = false;
        do
        {
            success = TagReads.TryRemove(tag.Epc, out TagInfo outTag);
        } while (!success);
    }
}

I feel like a dictionary is the correct choice here because there can be many tags to look up but something about this code nags me as being poor.

As far as efficiency goes. The speed is fine for now in our small scale test environment but I don't have a good way to find out how it will work on a massive scale until it is put to use, hence my concern.

Sach
  • 10,091
  • 8
  • 47
  • 84
Sam Marion
  • 690
  • 1
  • 4
  • 17
  • 1
    How big do you expect your dictionary to get? – Master Yoda Sep 01 '17 at 16:10
  • Why not just iterate over a list/array of `TagInfo` instead of a dictionary. You would have to content with collections not being able to be modified whilst enumerating, but you should have the same issue with iterating over a dictionary. It doesn't look like you are using anything from a dictionary that you can't do with a list or array – David Watts Sep 01 '17 at 16:11
  • 7
    I don't see any problem iterating Dictionary with `foreach` loop. However iterating it with `for` loop and highly inefficient `ElementAt` is definitely wrong. And where are the lookups btw? – Ivan Stoev Sep 01 '17 at 16:11
  • The dictionary can possibly get quite large and will vary widely. I would say its possible to have up to thousands of tags at a time if they aren't moving much. – Sam Marion Sep 01 '17 at 16:12
  • The reason I don't use a list is because tags will be read multiple times a second and I need to associate a time of when that tag was read and in what zone. I can post the code of when a tag is read if that would be helpful – Sam Marion Sep 01 '17 at 16:13
  • 1
    See this answer: https://stackoverflow.com/questions/3460729/is-there-a-limit-to-entries-in-a-dictionary. A few thousand shouldnt cause you trouble but as Ivan mentioned above you want to avoid using the for loop especially with ElementAt. – Master Yoda Sep 01 '17 at 16:14
  • Ok, great I'll look into getting rid of the element at. – Sam Marion Sep 01 '17 at 16:15
  • 1
    Side note: "I've read on here that ..." would be much better if you actually put link to what you actually read... – Alexei Levenkov Sep 01 '17 at 16:25
  • The purpose of a dictionary is similar to that of an indexed table in SQL. When you perform the query you are not iterating over each row just the rows that match your query, hence you dont consume as much memory and return results faster. In that regard whoever told you that iterating through a dictionary is abuse of it is to an extent correct. Again it depends on the scenario. – Master Yoda Sep 01 '17 at 16:29
  • _quite large_ is in the eye of the beholder. Some numbers would be more helpful.. – TaW Sep 01 '17 at 19:12

3 Answers3

3

If you are going to do a lot of lookup in your code and only sometimes iterate through the whole thing, then I think the dictionary use is ok. I would like to point out thought that your use of ElementAt is more alarming. ElementAt performs very poorly when used on objects that do not implement IList<T> and the dictionary does not. For IEnumerables<T> that do not implement IList the way the nth element is found is through iteration, so your for-loop will iterate the dictionary once for each element. You are better off with a standard foreach.

Titian Cernicova-Dragomir
  • 230,986
  • 31
  • 415
  • 357
  • What happens when a new tag is added to the dictionary while im iterating though? Tag reads can come in very quickly so I'm worried using a foreach will error if the collection is changed will iterating. – Sam Marion Sep 01 '17 at 16:14
  • Yes an exception can occur while you are iterating can happen if you are in a multithreaded scenario, you might want to consider `ConcurrentDictionary` if you are going to read and write at the same time. – Titian Cernicova-Dragomir Sep 01 '17 at 16:16
  • Yes this is a multi-thread scenario and already using one! :) – Sam Marion Sep 01 '17 at 16:17
  • 3
    Note that you are not safe from that with `ElementAt` as that just hides the iteration in the `ElementAt` method, the exception can still occur. – Titian Cernicova-Dragomir Sep 01 '17 at 16:17
  • If you are using `ConcurrentDictionary` I think you are safe to iterate, but you should check the documentation – Titian Cernicova-Dragomir Sep 01 '17 at 16:18
3

I believe that there's an alternative approach which doesn't involve iterating a big dictionary.

First of all, you need to create a HashSet<T> of tags on which you'll store those tags that have been detected in more than 2 zones. We'll call it tagsDetectedInMoreThanTwoZones.

And you may refactor your code flow as follows:

A. Whenever you detect a tag in one zone...

  1. Add the tag and the zone to the main dictionary.
  2. Create an exclusive lock against tagsDetectedInMoreThanTwoZones to avoid undesired behaviors in B..
  3. Check if the key has more than one zone. If this is true, add it to tagsDetectedInMoreThanTwoZones.
  4. Release the lock against tagsDetectedInMoreThanTwoZones.

B. Whenever you need to process a tag which has been detected in more than one zone...

  1. Create an exclusive lock against tagsDetectedInMoreThanTwoZones to avoid more than a thread trying to process them.
  2. Iterate tagsDetectedInTwoOrMoreZones.
  3. Use each tag in tagsDetectedInMoreThanTwoZones to get the zones in your current dictionary.
  4. Clear tagsDetectedInMoreThanTwoZones.
  5. Release the exclusive lock against tagsDetectedInMoreThanTwoZones.

Now you'll iterate those tags that you already know that have been detected in more than a zone!

In the long run, you can even make per-region partitions so you never get a tagsDetectedInMoreThanTwoZones set with too many items to iterate, and each set could be consumed by a dedicated thread!

Matías Fidemraizer
  • 63,804
  • 18
  • 124
  • 206
0

I feel like this is a good use for a dictionary, giving you good access speed when you want to check if an ID is already in the collection.

bic
  • 867
  • 1
  • 8
  • 21