views:

85

answers:

3

Is this a bad idea?

Private Class GH_DataStructureEnumerator(Of Q As Types.IGH_Goo)
  Implements IEnumerable(Of Q)
  Implements IEnumerator(Of Q)
  ....
  ....
  'Current, MoveNext, Reset etc.'
  ....
  ....

  Public Function GetEnumerator_Generic() As IEnumerator(Of Q) _
                  Implements IEnumerable(Of Q).GetEnumerator
    Return Me
  End Function
End Class

This class is only visible as an IEnumerable(Of T) readonly property, and it saves me an additional class that wraps IEnumerator(Of T). But somehow it just seems wrong. Is there a better way?

+2  A: 

Definitely a bad idea. In particular, it means that two calls to GetEnumerator will return references to the same object - when they should return independent iterators.

Now having said that, the C# compiler will generate classes which implement both types if you use iterator blocks... but it goes to great lengths to make sure that it gets it right. I suggest you don't put yourself through that pain :)

Jon Skeet
Jon, my life's goal is to avoid pain, thanks for the heads up.
David Rutten
+3  A: 

This is a bad idea because the solution will break down if someone attempts to enumerate your collection more than once at the same time.

For example: This will break in some way

For Each cur1 in yourObj 
  For Each cur2 in yourObj
    Console.WriteLine("{0} - {1}", cur1,cur2)
  Next
Next
JaredPar
Or even not at the same time, quite possibly :)
Jon Skeet
@Jon very true!
JaredPar
+1  A: 

From Implementing IEnumerable:

Do provide a GetEnumerator() method that returns a nested public struct called “Enumerator”.

By the way, this website is maintained by Brad Abrams - one of the authors of Framework Design Guidelines.

Andrew Hare
I've never liked this particular suggestion - it encourages mutable structs, which can behave in evil ways.
Jon Skeet
@Jon - That is a good point but I think that in this case it's not the worst idea. It's definitely a trade-off between performance (less memory pressure with a struct) and having a mutable struct which could introduce wierdness in the application. I think given the fact that most developers do not work with the enumerator instance directly that having it be a mutable struct is a good trade-off for the performance benefit. But in general I agree that mutable structs should be avoided.
Andrew Hare
@Andrew: Brad makes the trade-off very clear: Implementing this pattern involves having an extra public type (the Enumerator) and several extra public methods that are really there just for infrastructure reasons. These types add to the perceived complexity of the API and must be documented, tested, versioned, etc. **As such this pattern should only be followed where performance is paramount.** Consequently, this should *not* be used most of the time where performance is not too critical.
Mehrdad Afshari
@Mehrdad - Fair enough but I would argue that a type that implements `IEnumerator` doesn't really create _that_ much API complexity. Good point though :)
Andrew Hare