HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Indexed Property implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
indexedpropertyimplementation

Problem

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 : 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 : IIndexer
where TTarget : class
{
// Weak reference to target
private WeakReference _weaktarget;
// Get method
private Func _getter;
// Set method
private Action _setter;

public Indexer(TTarget target, Func getter, Action se

Solution

I wouldn't use it in production code.

I wouldn't use the static factory. If I have code that needs an IIndexer, 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 CreateIndexer(TTarget target,
                                            Func getter,
                                          Action 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
{
    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
{
    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 _variables;
public IIndexer 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 _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.

Code Snippets

interface IIndexerFactory
{
    IIndexer<TKey, TValue> CreateIndexer<TTarget, TKey, TValue>(TTarget target,
                                            Func<TTarget, TKey, TValue> getter,
                                          Action<TTarget, TKey, TValue> setter)
}
interface ISomething<TItem>
{
    string Something { get; }
    void Add(TItem item);
    TItem this[int index] { get; set; }
}
interface ISomethingElse
{
    string SomethingElse { get; }
    void Add(int value);
    int this[int index] { get; }
}
interface ISomethingWild<TItem>
{
    string SomethingWild { get; }
    void AddOrUpdate(TItem item);
    TItem this[int index = 0, string name = null] { get; }
}
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
    );

Context

StackExchange Code Review Q#125274, answer score: 4

Revisions (0)

No revisions yet.