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>.)

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){...}
–
graumanozMay 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.
–
TooToneMay 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.
–
graumanozMay 13 '13 at 10:52

5 Answers
5

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.

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:

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.

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

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.

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.