Take the 2-minute tour ×

I've been given some code from a customer that looks like this:

public class Thing
    // custom functionality for Thing...

public class Things : IEnumerable
    Thing[] things;
    internal int Count { get { return things.Length; } }

    public Thing this[int i] { get { return this.things[i]; } }

    public IEnumerator GetEnumerator() { return new ThingEnumerator(this); }

    // custom functionality for Things...

public class ThingEnumerator : IEnumerator
    int i;
    readonly int count;
    Things container;

    public ThingEnumerator(Things container)
        i = -1;
        count = container.Count;
        this.container = container;

    public object Current { get { return this.container[i]; } }
    public bool MoveNext() { return ++i < count; }
    public void Reset() { i = -1; }

What I'm wondering is whether it would have been better to have gotten rid of the ThingEnumerator class and replaced the Things.GetEnumerator call with an implementation that simply delegated to the array's GetEnumerator? Like so:

public IEnumerator GetEnumerator() { return things.GetEnumerator(); }

Are there any advantages to keeping the code as is? (Another thing I've noticed is that the existing code could be improved by replacing IEnumerator with IEnumerator<Thing>.)

share|improve this question
Your hunch is right. This code is old, before Generics came to c#. You can replace the GetEnumerator with public IEnumerable<Thing> GoForward() { foreach(var t in things) yield return t; } And to iterate over your class, use Things things = new Things(); foreach(var t in things){...} –  graumanoz May 13 '13 at 10:28
@graumanoz Thinking about it, this is probably old code that the customer has had knocking around a long time, and if they wrote similar from scratch they would use a more modern approach. –  TooTone May 13 '13 at 10:41
TooTone Yes, this is an old code :) It can be entirely replaced by the little method I've wrote before which is, by the way, the modern approach. –  graumanoz May 13 '13 at 10:52
add comment

5 Answers

up vote 2 down vote accepted

In the general case, there can sometimes be a reason to implement your own enumerator. You might want some functionality that the built-in one doesn't offer - some validation, logging, raising OnAccess-type events somewhere, perhaps some logic to lock items and release them afterwards for concurrent access (I've seen code that does that last one; it's odd and I wouldn't recommend it).

Having said that, I can't see anything like that in the example you've posted, so it doesn't seem to be adding any value beyond what IEnumerable provides. As a rule, if there's built-in code that does what you want, use it. All you'll achieve by rolling your own is to create more code to maintain.

share|improve this answer
add comment

The code you have looks like code that was written for .NET 1.0/1.1, before .NET generics were available - at that time, there was value in implementing your own collection class (generally derived from System.Collections.CollectionBase) so that the indexer property could be typed to the runtime type of the collection. However, unless you were using value types and boxing/unboxing was the performance limitant, I would have inherited from CollectionBase and there would be no need to redefine GetEnumerator() or Count.

However, now, I would recommend one of these two approaches:

  1. If you need the custom collection to have some custom functionality, then derive the collection from System.Collections.ObjectModel.Collection<Thing> - it provides all the necessary hooks for you to control insertion, replacement and deletion of items in the collection.

  2. If you actually only need something that needs to be enumerated, I would return a standard IList<Thing> backed by a List<Thing>.

share|improve this answer
add comment

An array enumerator does pretty much the same as your custom enumerator, so yes, you can just as well return the array's enumerator directly.
In this case, I would recommend you do it, because array enumerators also perform more error checking and, as you stated, it's just simpler.

share|improve this answer
add comment

Unless you are doing something truly custom (such as some sort of validation) in the custom enumerator, there really isn't any reason to do this no.

Generally, go with what is available in the standard libraries unless there is definite reason not to. They are likely better tested and have more time spent on them, as individual units of code, then you can afford to spend, and why recreate the wheel?

In cases like this, the code already exists but it may still be better to replace the code if you have time to test very well. (It's a no-brainer if there is decent unit test coverage.)

You'll be reducing your maintenance overhead, removing a potential source of obscure bugs and leaving the code cleaner than you found it. Uncle Bob would be proud.

share|improve this answer
add comment

With generics, there is really little value in implementing IEnumerable and IEnumerator yourself.

Removing these are replacing the class with a generic collection means you have far less code to maintain and has the advantage of using code that is known to work.

share|improve this answer
add comment

Your Answer


