Should an IEnumerable iterator on a Queue dequeue an item
Asked Answered
S

10

15

I have created a custom generic queue which implements a generic IQueue interface, which uses the generic Queue from the System.Collections.Generic namespace as a private inner queue. Example has been cleaned of irrelevant code.

public interface IQueue<TQueueItem>
{
    void Enqueue(TQueueItem queueItem);
    TQueueItem Dequeue();
}

public class CustomQueue<TQueueItem> : IQueue<TQueueItem>
{
    private readonly Queue<TQueueItem> queue = new Queue<TQueueItem>();
    ...
    public void Enqueue(TQueueItem queueItem)
    {
        ...
        queue.Enqueue( queueItem );
        ...
    }

    public TQueueItem Dequeue()
    {
        ...
        return queue.Dequeue();
        ...
    }
}

I want to keep things consistent with the core implementations and have noticed that the core Queue implements IEnumerable so I will do the same either by explicitly implementing IEnumerable on the class or inheriting it with the IQueue interface.

What I want to know is when enumerating over the queue should each move next dequeue the next item? I have used reflector to see how Microsoft has done it and all they do is step through the queues private array but Microsoft is far from infallible so I wanted to get a general opinion.

public class CustomQueue<TQueueItem> : IQueue<TQueueItem>, IEnumerable<TQueueItem>
{
    ...

    public IEnumerator<TQueueItem> GetEnumerator()
    {
        while (queue.Count > 0)
        {
            yield return Dequeue();
        }
    }

    //Or

    public IEnumerator<TQueueItem> GetEnumerator()
    {
        return queue.GetEnumerator();
    }

    ...
}

I am in two minds, on one hand I feel that iterating through a collection should not changed the collections state but on the other hand and especially with my particular implementation it would make the usage look clean.

EDIT

To put things into context. The class that I am implementing does a Monitor.Wait when Dequeuing and there are no items in the queue. When an item is put onto the queue there is a Monitor.Pulse. This allows for one thread to push stuff onto the queue and the other to essentially "watch" the queue.

From a coding point of view I am trying to decide which it looks cleaner:

foreach(QueueItem item in queue)
{
    DoSomethingWithThe(item);
}

//Or

while(systemIsRunning)
{
    DoSomethingWithThe(queue.Dequeue());
}

For my particular implementation it wouldn't matter if there were multiple process dequeuing items. Because it is a queue they can both pick of an item as no item should be processed more than once, hence the use of a queue.

EDIT

Interestingly enough I have found a blog post where someone has done exactly this.

Link

EDIT

One last stab at this before I close this off. How do people feel about the class not implement IEnumerable but having an IEnumerator GetEnumerator() method that dequeues items? The .net language supports duck typing, foreach being one of the uses. Perhaps this deserves it's own question?

EDIT

Have raised the question of implementing a GetEnumerator method without implementing IEnumerable in another question.

Sentimentalize answered 15/11, 2010 at 22:41 Comment(2)
I agree what I was considering doing was an abuse hence the reason for posting, I just needed to be told 10 or 11 times. I think the real question I have to ask myself is why do I need to implement IEnumerable? Am I doing it because the core implementation does as I have no real requirements for my queue to be iteratated over.Sentimentalize
Unfortunately I cannot mark you all as correct so I will give you all a day to clean up your answers and mark the highest voted as correct.Sentimentalize
C
20

Iterators should always be idempotent, that is, don't modify the queue as you iterate over it.

There are no guarantees that there won't be two concurrent iterations...


Edit to address your new comments:

When another programmer (such as your future self ;) ) comes along to add features to the code, they may not assume that the iterators are single-use. They might add a log statement that lists what's in the queue just before using it (oops).

Another thing I just thought of is that the visual studio debugger will often enumerate your classes for display. That would cause some extremely confusing bugs :)

If you're implementing a sub-interface of IEnumerable, and don't want to support IEnumerable, you should throw a NotSupportedException. Although this will not give you any compile time warnings, the run time error will be very clear, while a strange IEnumerable implementation could waste future you hours.

Crackbrain answered 15/11, 2010 at 22:43 Comment(7)
I agree that Iterators should be idempotent, I am just wondering if a queue is where you can break the mould. In my case (see edit) it wouldn't matter if there were multiple process iterating over the queue and in fact there probably will be.Sentimentalize
It's even simpler than a concurrency problem. IEnumerables are forward-only, read-only. Modifying the underlying concrete collection can cause IEnumerable implementations to get "lost", and so most of them don't allow it. Take a List, for example. Plug it into a foreach that removes each element. On the first go-round, index 0 disappears and index 1 becomes index 0. When the enumerator calls MoveNext, it will attempt to move from index 0 to index 1, which is now actually index 2. This will result in undesired behavior.Consciencestricken
@Keith in general that's an excellent point, but in the OP's code, the enumerator implementation doesn't exhibit that particular problem.Crackbrain
Additionally, one thing that I had not thought of till now is that I don't have to implement the IEnumerable inerface, all I have to do is provide a IEnumerator<TQueueItem> GetEnumerator() method as the .net language supports duck typing, foreach being one or the places. Now the question is this still an abuse?Sentimentalize
You might be interested to see some of the responses to my other question #4195400Sentimentalize
I am leery of any iterator that modifies the collection which it iterates over. I consider that to be the wrong way to do it, and will generally only accept it as valid when it is there is no choice but to do so. Jon Skeet gives examples where it might be necessary in his answer to your other question, though even in those situations my first instinct would be to try either avoid IEnumerable in favor of a Stream or to cache the response. But there are probably cases where using an iterator that modifies the collection could be superior.Amar
To avoid debugger/designer issues you could implement the necessary attributes that would hide that member from the debugger/designer. At least this would some issues if you wanted to throw an exception from GetEnumerator() or the like.Ber
B
12

Absolutely positively there is no way you should be mutating a collection as you iterate it. The whole point of iterators is that they provide a read-only non-destructive view of a collection. It would be deeply surprising to anyone using your code that looking at it changes it.

In particular: you do not want examining the state of your queue in the debugger to change it. The debugger calls IEnumerable just like any other consumer, and if that has side effects, they are executed.

Bridging answered 15/11, 2010 at 23:13 Comment(8)
You might be interested to see some of the responses to my other question #4195400Sentimentalize
-1 Why, then, does the BlockingCollection<T>.GetConsumingEnumerable Method remove and return items from the collection? It's absolutely positively mutating the collection as you iterate it.Alexipharmic
@ZaidMasud: When you understand the rules deeply and know how to break them safely, then you can do so.Bridging
@EricLippert I agree. I just thought the answer was strongly worded in a way that was categorically prohibiting this by saying that there is absolutely positively no way you should ever do this.Alexipharmic
@ZaidMasud: Do you think that the original poster understands the rules deeply enough to break them safely? If so then why are they asking the question?Bridging
@EricLippert no, but more people than the original poster will read this answer :) I am new to using BlockingCollection and originally thought that the GetConsumingEnumerable would not modify the underlying collection, in part because I had read your answer here. Therefore I was misled and wanted to point out that you might consider revising your wording :)Alexipharmic
@ZaidMasud: The name of the method GetConsumingEnumerable is designed to inform you that something unusual is going on.Bridging
@EricLippert cool so I think we're saying if you're going to break the rules, don't break them in the IEnumerable.GetEnumerator() implementation. Do so in another method that returns an IEnumerableAlexipharmic
B
5

I would suggest that you might want to have a method called something like DequeueAll which returns an item of a class that features a GetEnumerator method that represents everything in the queue, and clears the queue (if a queue item is added around the time the iEnumerable is created, the new item should either appear in the present all to AllItemsDequeued and not in the queue, or in the queue but not the present call). If this class implements iEnumerable, it should be constructed in such a way that the returned object will remain valid even after the an enumerator has been created and disposed (allowing it to be enumerated multiple times). If that would be impractical, it may be useful to give the class a name that suggests objects of the class should not be persisted. One could still do

foreach(QueueItem theItem in theQueue.DequeueAll()) {}
but would be less likely to (wrongfully) save the result of theQueue.DequeueAll to an iEnumerable. If one wanted maximum performance while allowing the result of theQueue.DequeueAll to be used as an iEnumerable, one could define a widening cast that would take a snapshot of the DequeueAll result (thus allowing the old items to be discarded).
Bradbury answered 15/11, 2010 at 23:15 Comment(1)
I like that, thinking outside the box.Sentimentalize
F
3

I am going to leave the bandwagon behind and say yes. This seems like a reasonable approach. Though I have to make one suggestion. That is, do not implement this consuming iterator in the GetEnumerator method. Instead call it GetConsumingEnumerator or something similiar. That way it is plainly obvious what is going to happen and the foreach mechanism will not be using it by default. You will not be the first person to do this. In fact, Microsoft already does this in the BCL via the BlockingCollection class and they even used GetConsumingEnumerator as the method1. I think you know what I am going to suggest next right?2

1How do you think I came up with the name suggestion? 2Why not just use BlockingCollection? It does everything you have asked for and more.

Fain answered 16/11, 2010 at 14:18 Comment(1)
I had noticed the IProducerConsumerCollection<T> interface before but I never noticed the BlockingCollection<T>, thanks for the heads up.Sentimentalize
E
2

I would say no, do it the second way.

Not only is this more consistent with the built-in Queue class, but it is also more consistent with the fact that the IEnumerable<T> interface is meant to be read-only.

Also, does this really seem intuitive to you?:

//queue is full
foreach(var item in queue)
    Console.WriteLine(item.ToString());
//queue is empty!?
Euclid answered 15/11, 2010 at 22:47 Comment(4)
I agree if that is how I was going to use it, use it is a bit of abuse and I think the question I really should be asking myself is should my queue really implement IEnumerable just because the core queue does. As there is no requirement for it I would have to say no.Sentimentalize
@Bronumski: Since it is a matter of only one line of code, I think the real question is, why not? I could certainly imagine the ability to enumerate (or especially use Linq on) a queue's elements to be useful, so I really don't see why you wouldn't.Euclid
Sure for a core API I could see it being useful but when developing from an Agile approach and accepting I am not going to abuse the language I am going to have to call YAGNI.Sentimentalize
@Bronumski: I think you are applying the agile slogan well out of context; for the enormous benefits you gain (including, as so many others have mentioned, debugging benefits), it is definitely worth considering. If this were an hour's worth of work, I could perhaps see the YAGNI argument as valid... but it's one very clean and repercussion-free line of code, which you already have written!Euclid
T
1

Strictly speaking, queue provides only push-back, pop-front, is-empty and maybe get-size operations. Everything that you add beside that is not a part of a queue, so if you decide to provide additional operations, you don't need to keep to the queue's semantics.

In particular, iterators are not a part of the standard queue interface, so you don't need to have them remove the item current being iterated. (As others have pointed out, this would contradict the expectations on iterators as well.)

Terceira answered 15/11, 2010 at 23:31 Comment(1)
I think that was part of my problem, I was looking at it from a purest point of view where a queue should only push and pop. Good explanation.Sentimentalize
H
0

Iterating through the queue should NOT dequeue.

This is designed to see examine the content of the Queue not to dequeue. This is also how MSMQ or MQ Series works.

Holp answered 15/11, 2010 at 22:44 Comment(0)
H
0

I don't think that it should. It would be very implicit, and it does not communicate that intent to anyone that is used to using the .net framework.

Hunsaker answered 15/11, 2010 at 22:44 Comment(0)
C
0

I'd say the second one. Your enumerator definitely should not change the state of the collection.

Cressler answered 15/11, 2010 at 22:46 Comment(0)
J
0

I had code where a propperty get was not idempotent... It was such a pain to decode. Please stick with the manual Dequeue.

Also you might not be the only one working on the queue, so it could get messy with more then one consumer.

Justajustemilieu answered 15/11, 2010 at 22:49 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.