7
\$\begingroup\$

As I wrapped the VBIDE API, I encountered a number of "collection types" (Windows, CodePanes, VBComponents, VBProjects, References, etc.) - these types implement the non-generic IEnumerable interface, so if I wanted to be able to iterate my wrappers like this for example:

using (var projects = _vbe.VBProjects)
{
    foreach (var project in projects.Where(p => p.Protection == ProjectProtection.Unprotected))
    {
        yield return project.Name;
    }
}

...then I was going to be needing some custom IEnumerator implementation, because even if I could explicitly .Cast<Rubberduck.VBEditor.DisposableWrappers.Project>() every time, at runtime the cast would fail because the enumerator yields a COM object, not a wrapper type.

So I made a generic ComWrapperEnumerator<TComCollection, TWrapperItem>, where TComCollection is the collection type (must implement IEnumerable) and TWrapperItem is the type of the SafeComWrapper<T> to be created and returned - am I doing this right? I don't like the way I'm assuming the constructor only has a single T parameter (where T is the COM type being wrapped by TWrapperItem), is there a better way?

using System;
using System.Collections;
using System.Collections.Generic;

namespace Rubberduck.VBEditor.DisposableWrappers
{
    public class ComWrapperEnumerator<TComCollection, TWrapperItem> : IEnumerator<TWrapperItem>
        where TComCollection : IEnumerable
    {
        private readonly IEnumerator _internal;

        public ComWrapperEnumerator(TComCollection items)
        {
            _internal = items.GetEnumerator();
        }

        public void Dispose()
        {
            var disposable = _internal as IDisposable;
            if (disposable == null)
            {
                return;
            }
            disposable.Dispose();
        }

        public bool MoveNext()
        {
            return _internal.MoveNext();
        }

        public void Reset()
        {
            _internal.Reset();
        }

        public TWrapperItem Current
        {
            get { return (TWrapperItem)Activator.CreateInstance(typeof(TWrapperItem), _internal.Current); }
        }

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

Anything else?

For context, here's the SafeComWrapper<T> abstract base class' constructor:

protected SafeComWrapper(T comObject)
{
    _comObject = comObject;
}

Here's how the enumerator is used, here in the CodePanes wrapper:

IEnumerator<CodePane> IEnumerable<CodePane>.GetEnumerator()
{
    return new ComWrapperEnumerator<Microsoft.Vbe.Interop.CodePanes, CodePane>(ComObject);
}

IEnumerator IEnumerable.GetEnumerator()
{
    return new ComWrapperEnumerator<Microsoft.Vbe.Interop.CodePanes, CodePane>(ComObject);
}

I can't think of a clean way to get generic type parameters inferred by the compiler, so every new ComWrapperEnumerator needs these ugly generic type parameters.

\$\endgroup\$
  • \$\begingroup\$ I vaguely remember reading about issues with IEnumerator not being disposable. Not sure if that was true or relevant. \$\endgroup\$ – RubberDuck Oct 4 '16 at 4:54
  • \$\begingroup\$ you must have been quite sleepy to write this code ;) \$\endgroup\$ – Bruno Costa Oct 4 '16 at 10:34
  • 1
    \$\begingroup\$ @BrunoCosta clearly this coffee needs more mug! \$\endgroup\$ – Mathieu Guindon Oct 4 '16 at 11:44
3
\$\begingroup\$

Your ComWrapperEnumerator is generic in two types without needing to. You don't really care about TComCollection aside that it must be an IEnumerable, thus that generic type can disappear from your class and you will end up with a constructor method that receives an IEnumerable.

public ComWrapperEnumerator(IEnumerable items)
{
    _internal = items.GetEnumerator();
}

Normally the non generic version calls the generic version of the method, removing your code repetition:

IEnumerator IEnumerable.GetEnumerator()
{
    //call the generic GetEnumerator
    return GetEnumerator();
}

In this particular case I think there isn't any other way to improve the instantion of ComWrapperEnumerator. However one way that normally is used to avoid specifying types on constructors is to specify a static method to instantiate the type. If you look for example to Tuple.Create this is exactly what they do.

The hack could work with code like this:

public static ComWrapperEnumerator<T> Create<T>(T t, IEnumerable value){
    return new ComWrapperEnumerator<T>(value);
}

//usage
ComWrapperEnumerator.Create(default(CodePane), ComObject);

You can always provide methods for all types that you care about:

public static ComWrapperEnumerator<CodePane> CreateCodePane(IEnumerable value){
    return new ComWrapperEnumerator<CodePane>(value);
}

But anyway, now that you have one less generic type maybe you are no longer concerned about that.

I don't like the way I'm assuming the constructor only has a single T parameter (where T is the COM type being wrapped by TWrapperItem), is there a better way?

Yes there is. Not only there is but would also remove the need of that pesky Activator.CreateInstace. The most straightforward approach that I can think of is to pass a Func that would receive the COM object and return an instance of the type you want. This would be a parameter of the constructor.

private readonly Func<object, TWrapperItem> _createItem;
public ComWrapperEnumerator(IEnumerable items, Func<object, TWrapperItem> createItem)
{
    _internal = items.GetEnumerator();
    _createItem = createItem;
}

public TWrapperItem Current
{
    get { return _createItem(_internal.Current); }
}

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

Obviously you can always fallback to Activator:

private TWrapperItem CreateItem(object value){
   return (TWrapperItem )Activator.CreateInstance(typeof(TWrapperItem), value);
}

public ComWrapperEnumerator(IEnumerable items, Func<object, TWrapperItem> createItem = null)
{
    _internal = items.GetEnumerator();
    _createItem = createItem ?? CreateItem;
}

Note that if you go with this approach you may consider to come back to having two generic types on your class, so you could have a typesafe _createItem.

\$\endgroup\$
  • \$\begingroup\$ return GetEnumerator() won't compile, because the IEnumerable and IEnumerable<T> interfaces are explicitly implemented, so either I make IEnumerator<CodePane> IEnumerable<CodePane>.GetEnumerator() a public member, or I cast this to IEnumerable<CodePane> in the non-generic GetEnumerator method. I think I prefer making the generic method an instance member, what do you think? \$\endgroup\$ – Mathieu Guindon Oct 4 '16 at 13:50
  • 2
    \$\begingroup\$ @Mat'sMug I think I would rather cast it. Making it an instance member expose details that you might not want to. \$\endgroup\$ – Bruno Costa Oct 4 '16 at 14:16
3
\$\begingroup\$

You rarely need to manually implement enumerators. In your case, you should probably just go with extension methods. For example:

public static class EnumerableEx
{
    public static IEnumerable<TTarget> Wrap<TTarget>(this IEnumerable source, Func<object, TTarget> converter)
    {
        return source.Cast<object>().Select(converter);
    }

    public static IEnumerable<TTarget> Wrap<TTarget>(this IEnumerable source)
    {
        return source.Wrap(x => (TTarget)Activator.CreateInstance(typeof (TTarget), x));
    }
}

P.S. I don't think there is a pretty way to dynamically convert one generic type to another. So using Activator is not a bad solution, especially since you can not modify source types. That being said, there are libraries, that do dynamic conversion using explicit cast operators (that you can define on your wrapper classes). I have no idea how on earth they do it (a lot of reflection involved?), but you can give them a try. Here is an example of such library, that I've used in the past.

\$\endgroup\$
  • \$\begingroup\$ @BrunoCosta, haha, you are right! Silly me :) \$\endgroup\$ – Nikita B Oct 4 '16 at 11:16
  • \$\begingroup\$ And I should say that I must have been quite sleepy to write my review. Just go with this approach, much simpler. \$\endgroup\$ – Bruno Costa Oct 4 '16 at 11:21
  • \$\begingroup\$ The problem is that the code base is already written, I just barely got the test project to build after swapping all VBIDE API references everywhere in Rubberduck so it's using the wrappers now - and without the enumerator I'll get invalid cast exceptions at runtime wherever I forget to use the extension method; I like this solution, but I think this is one such case where an enumerator makes sense. \$\endgroup\$ – Mathieu Guindon Oct 4 '16 at 11:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.