Indexed Property implementation

Question

I have a class that encapsulates a PowerShell Runspace object which, among many other things, has methods for getting and setting variables in the session state. There is no apparent mechanism for enumerating the variables, only get/set.
To simplify access to the variables from my code I've been using a specific indexer class. After I started writing another indexer for a different collection I thought it would be useful to have a generic implementation that would work for both and satisfy the DRY principle.

After several minor issues, much thought, discussion and testing I've come up with an indexer implementation that looks like it should be useful in a variety of situations and would like some input on what I can do to improve on it.

Design goals include:

• Flexible get/set via function delegates
• Weak referencing to allow proper object collection
• Lightweight interface
• Minimal exceptions - default values where possible
• As SOLID as feasible.

The interface for the class is:

public interface IIndexer<TKey, TValue> : IDisposable
{
    TValue this[TKey key] { get; set; }
}

I'm a little iffy about the IDisposable here. Initially I had nothing to dispose of but I want to be sure that any delegates are disconnected from the indexer in case they were closed around an object that might prevent garbage collection. Nulling the delegate references in the Dispose method is the best resolution I could find.

The Indexer class implements the above by holding a WeakReference to the object that holds the collection or otherwise grants access to whatever is being indexed, plus delegates to the methods that implement get and set on the indexer. Construction requires all three.

public class Indexer<TTarget, TKey, TValue> : IIndexer<TKey, TValue>
    where TTarget : class
{
    // Weak reference to target
    private WeakReference _weaktarget;
    // Get method
    private Func<TTarget, TKey, TValue> _getter;
    // Set method
    private Action<TTarget, TKey, TValue> _setter;

    public Indexer(TTarget target, Func<TTarget, TKey, TValue> getter, Action<TTarget, TKey, TValue> setter)
    {
        _weaktarget = new WeakReference(target);
        _getter = getter;
        _setter = setter;
    }

    public void Dispose()
    {
        _weaktarget = null;
        _getter = null;
        _setter = null;
    }

    public TValue this[TKey key]
    {
        get
        {
            // Get strong reference to target
            TTarget target = _weaktarget?.Target as TTarget;

            // invoke the get method if reasonable
            if (target != null && _getter != null)
                return _getter(target, key);
            // ... otherwise return a default value
            return default(TValue);
        }
        set
        {
            // Get strong reference to target
            TTarget target = _weakref?.Target as TTarget;

            // invoke set method if reasonable
            if (inst != null && _setter != null)
                _setter(target, key, value);
        }
    }
}

To aid in construction of the above I have a static IndexerFactory class. The CreateIndexer method both simplifies the signature (through type inference) and abstracts the implementation details from the user. The simple form is:

public static class IndexerFactory
{
    public static IIndexer<TKey, TValue> CreateIndexer<TTarget, TKey, TValue>
        (
            TTarget target,
            Func<TTarget, TKey, TValue> getter,
            Action<TTarget, TKey, TValue> setter
        )
        where TTarget : class
        => new Indexer<TTarget, TKey, TValue>(target, getter, setter);
}

I have considered having the indexer implementation(s) as private sub-class(es) of the IndexerFactory static class to increase the abstraction level. I may also use this class to define ToIndexer extensions for things like IDictionary<>. Discussions on these points defintely welcome.

Here's an example to show how this could be used:

public class Example : IDisposable
{
    private Dictionary<string, object> _variables;
    public IIndexer<string, object> Variables { get; private set; }

    public Example()
    {
        Variables = IndexerFactory.CreateIndexer
            (
                _variables,
                (inst, key) => 
                {
                    object result = null;
                    inst.TryGetValue(key, out result);
                    return result;
                },
                (inst, key, value) => inst[key] = value
            );
    }

    public void Dispose()
    {
        Variables.Dispose();
        Variables = null;
        _variables = null;
    }
}

I'm interested in any weaknesses you can find in the above code, discussions about the merits of the points mentioned above and any improvements that you can think of.

Answer 1

I wouldn't use it in production code.

I wouldn't use the static factory. If I have code that needs an IIndexer<TKey,TValue>, and I'm writing SOLID code, I'm not going to want to couple it with any static dependency. Instead if I need a factory, I'll constructor-inject an abstract factory interface:

interface IIndexerFactory
{
    IIndexer<TKey, TValue> CreateIndexer<TTarget, TKey, TValue>(TTarget target,
                                            Func<TTarget, TKey, TValue> getter,
                                          Action<TTarget, TKey, TValue> setter)
}

That way I'll be able to call _factory.CreateIndexer(typeof(Foo), ..., ...); whenever I need to. And then the factory dependency is controlled from the outside - when you write unit tests for that type, you inject a factory implementation that's completely under your control.

The thing is, whether a type exposes an indexer or not, is pretty much an implementation detail, a little API sugar for your code. If you're coding against interfaces and happen to feel the need for an indexer on an interface, nothing forbids being specific about it:

interface ISomething<TItem>
{
    string Something { get; }
    void Add(TItem item);
    TItem this[int index] { get; set; }
}

Other times what I need is this:

interface ISomethingElse
{
    string SomethingElse { get; }
    void Add(int value);
    int this[int index] { get; }
}

Or perhaps this:

interface ISomethingWild<TItem>
{
    string SomethingWild { get; }
    void AddOrUpdate(TItem item);
    TItem this[int index = 0, string name = null] { get; }
}

Your IIndexer assumes I'll need a setter method for that indexer, and that I'll want to access it with one parameter, of a known given type.

---

IDisposable always annoys me on an interface. It forces all implementers to be disposable, or, well, to at least appear to be. That's IDeceptive right there. I think that's a nice example of a leaky abstraction, where a specific implementation leaks into the abstraction and paints you in a corner.

Indexers are just a tool in one's arsenal, to write code that's easier to work with. When there's a type I'd like an indexer for, I write an indexer for it: that indexer works closely with that type, it knows it inside out - it is the type. Why make it its own interface? An indexer is a member of a type... not the interface.

---

Deep down, between this:

private Dictionary<string, object> _variables;
public IIndexer<string, object> Variables { get; private set; }

Variables = IndexerFactory.CreateIndexer
    (
        _variables,
        (inst, key) => 
        {
            object result = null;
            inst.TryGetValue(key, out result);
            return result;
        },
        (inst, key, value) => inst[key] = value
    );

And this:

private Dictionary<string, object> _variables;
public object this[string value] { get { return _variables[value]; } }

I'd go for the one that looks like an indexer and doesn't introduce a new dependency; I'll keep a clean constructor, and I'll only implement a setter if I need one, and I have no concerns about IDisposable.

Sometimes it's best to stick with the basics - the KISS principle:

Keep It Simple Stupid.

Answer 2

If a getter can throw an exception, it should probably be redesigned to be a method. Notice that this rule does not apply to indexers, where we do expect exceptions as a result of validating the arguments.

^{_{Property design guidelines, MSDN}}

Your implementation doesn't follow the property design guidelines for indexers. More specifically, here:

// invoke the get method if reasonable
if (target != null && _getter != null)
    return _getter(target, key);
// ... otherwise return a default value
return default(TValue);

otherwise return a default value is violating the Principle of Least Surprise/Astonishment; if I give the indexer a value that I know doesn't exist, I expect no less than an exception to be thrown.

Your code will throw that expected exception, but will happily return a default(TValue) when the getter or target is null - I'd probably expect an InvalidOperationException there:

if (target != null && _getter != null)
{
    return _getter(target, key);
}
throw new InvalidOperationException();

...but since we're throwing an exception anyway, I'd probably just let the framework throw a NullReferenceException... which brings us to fail early - what if the constructor validated for a non-null getter?

Make the setter optional and defaulted to null, and throw an ArgumentNullException when the target or the getter function is null - and then you can do away with that null check altogether:

public Indexer(TTarget target, Func<TTarget, TKey, TValue> getter, Action<TTarget, TKey, TValue> setter = null)
{
    if (target == null) throw new ArgumentNullException(nameof(target));
    if (getter == null) throw new ArgumentNullException(nameof(getter));

    _weaktarget = new WeakReference(target);
    _getter = getter;
    _setter = setter;
}

The get call becomes quite straightforward:

    get
    {
        TTarget target = _weaktarget?.Target as TTarget;
        return _getter(target, key);
    }

That would of course rule out set-only indexers... but those would be weird anyway, right?