CODEWITHSUNDEEP
c#
TooTone
TooTone

Reputation: 8146

Should I define custom enumerator or use built-in one?

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

Upvotes: 3

Views: 588

Answers (5)

anaximander
anaximander

Reputation: 7140

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.

Upvotes: 2

Jean Hominal
Jean Hominal

Reputation: 16796

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

Upvotes: 2

Botz3000
Botz3000

Reputation: 39630

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.

Upvotes: 1

Frater
Frater

Reputation: 710

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.

Upvotes: 1

Oded
Oded

Reputation: 499172

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.

Upvotes: 3

Related Questions