Are there any tools that can help us refactor IEnumerator properties to IList or similar?

Question

We have a very old code base(that actually is not horrible quality). It dates back to when .Net was pre-release, which I suspect is the cause of some of these weird conventions.

Anyway, we just began to drop .Net 1.1 support and are having a hay-day with converting things to generics and using Linq and all that fun stuff. One of the most annoying patterns in our code base though is we'll have something like

private ArrayList mylist;
public IEnumerator MyList
{
  get
  {
    if(mylist==null)
      return new EmptyEnumerator.Enumerator;
    return mylist.GetEnumerator();
  }
}

This pattern is particularly horrible because it prevents us from simply doing foreach(var item in MyList) because IEnumerator doesn't implement IEnumerable. Instead we must do something like this:

IEnumerator enumerator=MyList;
while(enumerator.MoveNext())
{
    object item=enumerator.Current;
}

So, for refactoring, we of course want to use something like ReadOnlyCollection<T> or IList<T> or similar. To do this however, we have to update every single reference to MyList to do:

IEnumerator enumerator=MyList;

to

IEnumerator enumerator=MyList.GetEnumerator();

In some cases, we can have over a hundred references to one property. Are there any tools out there that can make this easier? We recently got Resharper(not for this issue, but just for general use), but it doesn't appear to cover this type of scenario.

Why not fix up the *uses* - instead of keeping calling `GetEnumerator()` manually? I would expect most of them would end up being `foreach` loops anyway, right? — Jon Skeet, Oct 19 '12 at 20:36
Are the usages consistent enough that you could regex or write a quick script to read all the .cs files and find/replace the pattern? — Chris Sinclair, Oct 19 '12 at 20:37
@JonSkeet We'd love to, but we don't have a month to dedicate to the task :) So our plan right now is to fix the root cause now so we can write good new code, and fix old code as we come across it — Earlz, Oct 19 '12 at 20:38
@Earlz Could you have two properties, one that's an IEnumerator (the existing one) and one that's an `IEnumerable` with a similar name. New uses use the new property, old ones use the old one. Refactor out uses of the old one as you come across it, and then remove it entirely when you can. — Servy, Oct 19 '12 at 20:40
Of course there are some weird things where manual intervention is required, like when a method's argument is an IEnumerator and such as well... But, for the most part, it's usually like this. @ChrisSinclair I can't think of how you'd use a Regex to solve this problem, unless you did it one property at a time and was really really sure that an individual name is only used in that one place — Earlz, Oct 19 '12 at 20:41
@Servy we could, but would rather not. I mean, what do you use for a property called `Assemblies`? We'd rather keep our property names consistent — Earlz, Oct 19 '12 at 20:42
@Earlz But if it follows a pattern like `IEnumerator enumerator=XXXXX;` you could probably replace a lot of cases and manually double-check and fix up the ones missed, unless your code in use is more varied than this? — Chris Sinclair, Oct 19 '12 at 20:43
@ChrisSinclair true, but that sounds rather risky.. Though, thinking about it like that makes it actually sound possible, so maybe. We'd also have to go the "all or nothing" approach with that — Earlz, Oct 19 '12 at 20:44
@Servy Probably significantly more than a hundred.. This is why it'd take a lot of time to do if we did such a thing all at once — Earlz, Oct 19 '12 at 20:45
@Earlz: Write the script to first show you the changes it intends to make. Scan through the list, make sure it's good, then rock on! (make sure you have a copy of the code/code versioning!) Even if it handles 75% of the work, you can fix the rest. — Chris Sinclair, Oct 19 '12 at 20:47
@ChrisSinclair The reason it's all or nothing is we'd have to update not only the USE statements, but this would force us to also update every single property definition, which can be time consuming when deciding what is the best interface to allow for each property — Earlz, Oct 19 '12 at 20:49

score 4 · Answer 1 · answered Oct 19 '12 at 20:48

4

What it sounds like you need to do is return a class that implements both IEnumerator and IEnumerable<T>

It's not actually that hard to just make your own type that does this:

public class MessedUpIterator<T> : IEnumerable<T>, IEnumerator
{
    private IEnumerable<T> source;
    private IEnumerator enumerator;

    private IEnumerator MyEnumerator
    {
        get
        {
            return enumerator ?? source.GetEnumerator();
        }
    }

    public MessedUpIterator(IEnumerable<T> source)
    {
        this.source = source;
    }
    public IEnumerator<T> GetEnumerator()
    {
        return source.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return source.GetEnumerator();
    }

    object IEnumerator.Current
    {
        get { return MyEnumerator.Current; }
    }

    bool IEnumerator.MoveNext()
    {
        return MyEnumerator.MoveNext();
    }

    void IEnumerator.Reset()
    {
        MyEnumerator.Reset();
    }
}

Now instead of returning either an IEnumerator or an IEnumerable<T> you can return something that does both.

Note that IEnumerator is implemented explicitly while IEnumerable<T> is implemented implicitly, so it will encourage using it as an IEnumerable, while still make using it as an IEnumerator possible.

Yes, it's ugly, but it certainly could be worse.

answered Oct 19 '12 at 20:48

Servy

202,030
26
332
449

We're trying to make our code better, not more hackish. Also, if we decide to allow something like `IList` because a piece of code needs it's additional functions instead of IEnumerable, this solution will fall through – Earlz Oct 19 '12 at 20:51
@Earlz It would be easy enough to make a class that implemented both `IList` and `IEnumerator`. Since `IList` extends `IEnumerable` it would be a pretty trivial change; mostly just making stubs for each `IList` method that call the method on an `IList` stored internally. As for it being hackish, you ruled out *actual* fixes to the problem by specifying that caller code not be changed. – Servy Oct 19 '12 at 20:54
Well, the caller code can be changed, just not drastically. Hence why I was looking for some way to automatically append `.GetEnumerator()` to each reference of a property – Earlz Oct 19 '12 at 20:55
1

@Earlz This might be a good stepping stone though. Implement this for your close release, then do a point-release after with everything done _correctly_. Either that or you might have to resort to the most fundamental tool of all over an upcoming weekend: bloody fingertips. – Chris Sinclair Oct 19 '12 at 20:56
1

Chris has it right. You use something like this for the short term, so that you are *able* to use the correct method for new code. Then, when you have the available resources, go in and just change this type to not implement `IEnumerator`. Then just go around fixing code everywhere there's a syntax error. When you run out of time, fix this class so everything compiles. When you have fixed it all then you can remove this type entirely, or just commit the change to not implement `IEnumerator`. – Servy Oct 19 '12 at 21:04

Are there any tools that can help us refactor IEnumerator properties to IList or similar?

1 Answers1