5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I've made a couple (seemingly minor) edits to your question. Feel free to roll back or edit them out as appropriate. (I put the example code block in a quote block, as that is how we denote on this site that the block is not up for review.) Hopefully you get some good reviews. :) \$\endgroup\$ – Der Kommissar Apr 10 '16 at 4:16
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. Regarding read-only indexers, that is a trivial modification to the code (additional interface, same implementation). Multidimensional indices are doable too. For indexers being part of the type they index, that is not always the case and I don't always have the ability to modify the types that I want to index. In the case of Runspace the variables hide a couple levels down in Microsoft's objects that I have no control over. \$\endgroup\$ – Corey Apr 10 '16 at 8:43
  • \$\begingroup\$ Regarding the static factory, it seems to add unnecessary complexity to try to abstract that as it performs a very simple function that is wholly internal to the Indexer code and should never need to be mocked for testing or changed to suit a particular situation. What it does is add some abstraction to the construction of interfaces for their consumers. If I never need a factory implementation that is different to what is shown above, why complicate things? \$\endgroup\$ – Corey Apr 10 '16 at 8:50
  • \$\begingroup\$ On IDisposable, the point here is that Indexers need to tidy up after themselves to avoid holding references to objects and thus tying them up and stopping the GC from dealing with them. Forcing IDisposable on the implementations prompts the implementer to do this. Besides, how does the class that instantiates this type dispose of it if the interface doesn't require IDisposable? Do I have to check every instance for an IDisposable compatibility every time I use one? That's not keeping it simple, that's adding complexity. \$\endgroup\$ – Corey Apr 10 '16 at 8:55
  • \$\begingroup\$ If the solution looks like a generic "indexer-for-anything", then it can be assumed that it can be used as such - and imo it's not a good idea. If it's meant to be for a specific type, then it should look like it - I don't see the problem with making an IndexedRunspace instead, via composition or inheritance (assuming the extended type isn't sealed), that exposes indexers as needed. That solves the IDisposable issue btw, since the indexer isn't holding on to anything when it's a member of the disposable type in question. \$\endgroup\$ – Mathieu Guindon Apr 10 '16 at 14:23
  • \$\begingroup\$ Thanks for your feedback Mat. I will add a read-only indexer interface and look at adding multidimensional support at a later time. If you have any further constructive critique I would be keen to hear it. I'm not particularly interested in reasons not to use this code, since I have already concluded that the alternatives are more costly and less simple. DI and abstract factories for instance are certainly not KISS, nor is composition of multiple rich sealed classes to provide a single indexer property. \$\endgroup\$ – Corey Apr 11 '16 at 8:37
4
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Nice catch. In this case the result should be an IndexOutOfRange exception or similar? I'll add that to the implementation... will update the code in the question when I get some spare time. \$\endgroup\$ – Corey Apr 11 '16 at 23:31
  • \$\begingroup\$ @Corey don't update the code in the question - ask a new one instead (see here for more info). That said, yeah an IndexOutOfRangeException would be ideal imo. \$\endgroup\$ – Mathieu Guindon Apr 11 '16 at 23:34
  • \$\begingroup\$ @Corey re-reading again, turns out your indexer does throw that expected exception - the default(TValue) is only returned when the target or the getter is null: I've edited this answer accordingly. \$\endgroup\$ – Mathieu Guindon Apr 12 '16 at 13:56
  • 1
    \$\begingroup\$ Throwing an exception on a bad index is left up to the getter/setter methods to implement since they will determine what is and isn't a bad index. There are some exceptions that legitimately should be thrown in the Indexer implementation though - ObjectDisposedException for one since a) I'm implementing IDisposable and b) the referenced target could be destroyed without references to the Indexer instance being cleaned up. Also possible InvalidOperationException if a read-only indexer is recast and someone attempts to set a value through it. \$\endgroup\$ – Corey Apr 13 '16 at 1:49

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.