Showing posts with label Code Quality. Show all posts
Showing posts with label Code Quality. Show all posts

Saturday, January 11, 2014

ReplaySubject Performance improvments – code changes

In the previous post I talked about some changes that could be made to the ReplaySubject<T> implementation to squeeze large performance gains out of it. In that post I discussed mainly the result of the changes. In this post I will show some of the changes that I made.

Analysis of existing implementation

As mentioned in the previous post, all constructor overloads of the ReplaySubject<T> eventually called into the same constructor, but just provided default values for the MaxCountBuffer, MaxTimeBuffer and Scheduler. For example :

public ReplaySubject(int bufferSize, TimeSpan window, IScheduler scheduler)
{
    _bufferSize = bufferSize;
    _window = window;
    _scheduler = scheduler;

    _stopwatch = _scheduler.StartStopwatch();
    _queue = new Queue<TimeInterval<T>>();
    _isStopped = false;
    _error = null;

    _observers = new ImmutableList<ScheduledObserver<T>>();
}

public ReplaySubject(int bufferSize, TimeSpan window)
    : this(bufferSize, window, SchedulerDefaults.Iteration)
{ }

public ReplaySubject()
    : this(InfiniteBufferSize, TimeSpan.MaxValue, SchedulerDefaults.Iteration)
{ }

public ReplaySubject(IScheduler scheduler)
    : this(InfiniteBufferSize, TimeSpan.MaxValue, scheduler)
{ }

public ReplaySubject(int bufferSize, IScheduler scheduler)
    : this(bufferSize, TimeSpan.MaxValue, scheduler)
{ }

public ReplaySubject(int bufferSize)
    : this(bufferSize, TimeSpan.MaxValue, SchedulerDefaults.Iteration)
{ }

public ReplaySubject(TimeSpan window, IScheduler scheduler)
    : this(InfiniteBufferSize, window, scheduler)
{ }

public ReplaySubject(TimeSpan window)
    : this(InfiniteBufferSize, window, SchedulerDefaults.Iteration)
{ }

There are a total of 8 constructor overloads, but 7 of them just delegate to the top overload above, passing default values.

Next, lets look at the Subscribe method. Here we see the passed observer is wrapped in a ScheduledObserver<T>, which is only relevant for time based buffers. Also a Trim() command is made which again is only relevant for time based buffers.

public IDisposable Subscribe(IObserver observer)
{
    var so = new ScheduledObserver(_scheduler, observer);
    var n = 0;
    var subscription = new RemovableDisposable(this, so);
    lock (_gate)
    {
        CheckDisposed();
        Trim(_stopwatch.Elapsed);
        _observers = _observers.Add(so);

        n = _queue.Count;
        foreach (var item in _queue)
            so.OnNext(item.Value);

        if (_error != null)
        {
            n++;
            so.OnError(_error);
        }
        else if (_isStopped)
        {
            n++;
            so.OnCompleted();
        }
    }
    so.EnsureActive(n);
    return subscription;
}

The next part of the code that is interesting is the implementation of the OnNext method.

public void OnNext(T value)
{
    var o = default(ScheduledObserver<T>[]);
    lock (_gate)
    {
        CheckDisposed();

        if (!_isStopped)
        {
            var now = _stopwatch.Elapsed;
            _queue.Enqueue(new TimeInterval<T>(value, now));
            Trim(now);

            o = _observers.Data;
            foreach (var observer in o)
                observer.OnNext(value);
        }
    }

    if (o != null)
        foreach (var observer in o)
            observer.EnsureActive();
}

There are several things to note here:

  1. The use of an array of ScheduledObserver<T>
  2. The use of the TimeInterval<T> envelope for the value
  3. The Trim() command

Each of the three things above become quite dubious when we consider ReplayAll and ReplayOne implementations. A ReplayMany implementation, may need a Trim() command, but surely does not need ScheduledObservers nor its values time-stamped.

Next we look at the Trim command itself:

void Trim(TimeSpan now)
{
    while (_queue.Count > _bufferSize)
        _queue.Dequeue();
    while (_queue.Count > 0 && now.Subtract(_queue.Peek().Interval).CompareTo(_window) > 0)
        _queue.Dequeue();
}

Again we see that the code for the second while loop is wholly unnecessary for non-time based implementations.

Each of these small overheads start to add up. However where the cost really start to kick in is in the implementation of the ScheduledObserver<T>.

At a minimum, for each OnNext call to a ReplaySubject<T>, the ScheduledObserver<T> performs some condition checks, virtual method calls and some Interlocked operations. These are all fairly cheap operations. It is the unnecessary scheduling that incurs the costs. The scheduling incurs at least the following allocations

  1. ScheduledItem<TimeSpan, TState>
  2. AnonymousDisposable and the Action
  3. IndexedItem

There is also all the queues that are involved.

  1. ReplaySubject<T>._queue (Queue<TimeInterval<T>>)
  2. ScheduledObserver<T>._queue (ConcurrentQueue<T>)
  3. CurrentThreadScheduler.s_threadLocalQueue (SchedulerQueue<TimeSpan>)

And the concurrency controls

  1. ReplaySubject uses the IStopWatch from _scheduler.StartStopwatch()
  2. ScheduledObserver<T>; will use Interlocked.CompareExchange 3-4 times on a standard OnNext call.
  3. ConcurrentQueue uses a Interlocked.Increment and also a System.Threading.SpinWait when en-queuing a value. It also uses up to 3 SpinWaits and a Interlocked.CompareExchange to de-queue a value.
  4. The recursive scheduling extension method use lock twice
  5. CurrentThreadScheduler uses a Thread.Sleep()

Alternative implementations

All of the code above is fairly innocent looking when looked at in isolation. However, when we consider what we probably need for a ReplayOne, ReplayMany or ReplayAll implementation, all this extra code might make you blush.

The implementations that do not have a time consideration now share a base class and its updated OnNext implementation is now simply:

public void OnNext(T value)
{
    lock (_gate)
    {
        CheckDisposed();

        if (!_isStopped)
        {
            AddValueToBuffer(value);
            Trim();

            var o = _observers.Data;
            foreach (var observer in o)
                observer.OnNext(value);
        }
    }
}

The ReplayOne implementation is now reduced to :

private sealed class ReplayOne : ReplayBufferBase, IReplaySubjectImplementation
{
    private bool _hasValue;
    private T _value;

    protected override void Trim()
    {
        //NoOp. No need to trim.
    }

    protected override void AddValueToBuffer(T value)
    {
        _hasValue = true;
        _value = value;
    }

    protected override void ReplayBuffer(IObserver<T> observer)
    {
        if (_hasValue)
            observer.OnNext(_value);
    }

    protected override void Dispose(bool disposing)
    {
        base.Dispose(disposing);
        _value = default(T);
    }
}

Note that there are no queues, schedulers, allocations etc. We have replaced all of that with simply a field to hold the single value, and a boolean flag to indicate if there has been a value buffered yet.

This is allowed to become so simple due to the base class ReplayBufferBase:

private abstract class ReplayBufferBase : IReplaySubjectImplementation
{
    private readonly object _gate = new object();
    private bool _isDisposed;
    private bool _isStopped;
    private Exception _error;
    private ImmutableList<IObserver<T>> _observers;

    protected ReplayBufferBase()
    {
        _observers = new ImmutableList<IObserver<T>>();
    }

    protected abstract void Trim();
    protected abstract void AddValueToBuffer(T value);
    protected abstract void ReplayBuffer(IObserver<T> observer);

    public bool HasObservers
    {
        get
        {
            var observers = _observers;
            return observers != null && observers.Data.Length > 0;
        }
    }

    public void OnNext(T value)
    {
        lock (_gate)
        {
            CheckDisposed();

            if (!_isStopped)
            {
                AddValueToBuffer(value);
                Trim();

                var o = _observers.Data;
                foreach (var observer in o)
                    observer.OnNext(value);
            }
        }
    }

    public void OnError(Exception error) {/*...*/}

    public void OnCompleted() {/*...*/}

    public IDisposable Subscribe(IObserver<T> observer) {/*...*/}

    public void Unsubscribe(IObserver<T> observer) {/*...*/}

    private void CheckDisposed() {/*...*/}

    public void Dispose() {/*...*/}

    protected virtual void Dispose(bool disposing) {/*...*/}
}

The ReplayMany and ReplayAll implementations are slightly more complex as they require a Queue to store the buffered values. Again we add another base class to do most of the work.

private abstract class ReplayManyBase : ReplayBufferBase, IReplaySubjectImplementation
{
    private readonly Queue<T> _queue;

    protected ReplayManyBase(int queueSize)
        : base()
    {
        _queue = new Queue<T>(queueSize);
    }

    protected Queue<T> Queue { get { return _queue; } }

    protected override void AddValueToBuffer(T value)
    {
        _queue.Enqueue(value);
    }

    protected override void ReplayBuffer(IObserver<T> observer)
    {
        foreach (var item in _queue)
            observer.OnNext(item);
    }

    protected override void Dispose(bool disposing)
    {
        base.Dispose(disposing);
        _queue.Clear();
    }
}

Now the only differences are the initial buffer size and whether the buffer gets trimmed or not. This leaves us with the final two implementations:

private sealed class ReplayMany : ReplayManyBase, IReplaySubjectImplementation
{
    private readonly int _bufferSize;

    public ReplayMany(int bufferSize)
        : base(bufferSize)
    {
        _bufferSize = bufferSize;
    }

    protected override void Trim()
    {
        while (Queue.Count > _bufferSize)
            Queue.Dequeue();
    }
}

private sealed class ReplayAll : ReplayManyBase, IReplaySubjectImplementation
{
    public ReplayAll()
        : base(0)
    {
    }

    protected override void Trim()
    {
        //NoOp; i.e. Dont' trim, keep all values.
    }
}

Less code, more speed

Like Kunu says "Do less".

By removing a lot of the excess code we are able to massively improve the performance of the ReplaySubject<T> for arguably most use-cases.

Saturday, June 18, 2011

Isolating custom Library dependencies versions from consumer dependency versions

This post is more about the CLR and dependency management than it is about Rx. However at this point in Rx’s lifecycle it seems relevant to comment on. The same principles could obviously be applied to open source projects et. al.

I have recently heard of an interesting situation that no-doubt has proved troublesome to people before. This problem is particularly interesting with Rx and it’s recent rate of versions. If you are trying to incorporate Rx into a library of yours and you then want to publish that library, you are effectively forcing users to use the same version of Rx that you do. Considering that a version of Rx comes out say every two months and that often there are breaking changes, this can create quite a mess. It is also made more interesting that up until recently they have not specified if a release was experimental or considered stable.

To provide an example to better understand my particular point; imagine you have a library that wraps a messaging platform. You want to avoid the use of Events and APM, and expose things via IObservable<T>. You feel happy that IObservable<T> is exposed natively in .NET 4 so you should not have to expose your implementations of Rx. You do however, want to use Rx as it has features you need and don’t want to (re-)write yourself. Your standard approaches to package/deployment are:

  • deploy the parts of the Rx libraries you use with your code. Users can just put them all into a “lib\MyFramework” folder and reference them.
  • excluded the libraries from your code and set Specific Version = False and hope your code will work with the consumer’s version of Rx
  • use a package management tool like Nuget to publish your package and specify the valid versions of Rx your library will work with.
  • rely on things being in the GAC so you can utilise the side-by-side versioning it provides
  • just try and implement the parts of Rx you want and avoid DLL Hell.

I gave this some thought and I think I have come up with a solution that could help library authors protect themselves. While there is the obvious option of using Nuget as part of your dependency management, this does not solve the problem, it just eases the pain. If the customer wants to use the latest version of Rx and you only support the 3 previous versions, your customer is still in some trouble.

The theory i had was that I can specify the specific version of Rx I want to reference in my library, the problem being that it may be named the same as the client’s referenced version. Depending on where their references were built to, they could overwrite each other. It seemed the solution was to embed the dependency into my library.

This turns out to actually be quite easy. If you have a project in visual studio that references your version of Rx, you have to follow these steps:

  1. Ensure you have a file reference (not a project or a GAC reference) to the dependency, in this case System.Reactive.dll
  2. Set the reference to be Specific Version = True
  3. Set the reference Copy Local = False
  4. Embed the dependency into your library. I created a folder in my project called EmbeddedAssemblies. I “Add Existing…” to this folder, navigate to the dependencies (just System.Reactive.dll in this case), and then choose “Add as Link…
  5. Set the “Build Action” of  the newly added link to Embedded Resource
  6. Ensure that the resource is loaded correctly at run time…

The last part of that list proves to be not too hard. You can hook on to the AppDomain.AssemblyResolve event and load your embedded resource. You can return your embedded dependency by reading the byte stream and creating the assembly from it and then returning that in the event handler

internal static class DependencyResolver
{
  private static int _isSet = 0;

  internal static void Ensure()
  {
    if (Interlocked.CompareExchange(ref _isSet, 1, 0) == 0)
    {
      var thisAssembly = Assembly.GetExecutingAssembly();
      var assemblyName = new AssemblyName(thisAssembly.FullName).Name;
      var embededAssemblyPrefix = assemblyName + ".EmbeddedAssemblies.";

      var myEmbeddedAssemblies =
        Assembly.GetExecutingAssembly().GetManifestResourceNames()
          .Where(name => name.StartsWith(embededAssemblyPrefix))
          .Select(resourceName =>
                    {
                      using (var stream = Assembly.GetExecutingAssembly().GetManifestResourceStream(resourceName))
                      {
                        var assemblyData = new Byte[stream.Length];
                        stream.Read(assemblyData, 0, assemblyData.Length);
                        return Assembly.Load(assemblyData);
                      }
                    })
          .ToDictionary(ass => ass.FullName);

      AppDomain.CurrentDomain.AssemblyResolve += (sender, args) =>
                                                    {
                                                      Assembly assemblyToLoad = null;
                                                      myEmbeddedAssemblies.TryGetValue(args.Name, out assemblyToLoad);
                                                      return assemblyToLoad;
                                                    };
    }
  }
}

You can return null in the event handler to say I don't know how to load this assembly. This allows others to be able to have a go at loading the assembly in the same way.

Next, to ensure that my embedded dependency resolver is called, I opted to make a call to it in the static constructor of the key types in my library. eg

public class SomeProvider
{
  static SomeProvider()
  {
    DependencyResolver.Ensure();
  }

  //other stuff goes here...
}

Have a look at the example code by downloading this zip file

Caveats : This is a thought and the code and concepts are only demo quality. I have not used this in production quality code. I also have not verified that the license allows for this style of packaging.

Further links:

Sunday, February 13, 2011

Silverlight testing

I am putting this out there to see if I can get some traction with other Test Driven Silverlight coders out there. If you are one of these people you will know of the strife your day-to-day coding. For those who don't know what I mean these are the three options a Silverlight developer has with regards to test driven coding:

1) Use the Silverlight Unit Test Framework. The problem with this is that you lose any integrated development support, it is amazingly slow (in the area of 1-5 tests per second), and doesn't have any useful build tool support (coverage, TFS, TeamCity). Massive Fail.

2) Cross compile to .NET 4 all of your Models, ViewModels, Controllers, Modules, Presenters (i.e. everything that is not a View or a Control). Now write unit tests against this project. This means you get back to fast tests (100s tests per second) but take a hit on compiling twice and managing the project linking and just having twice as many projects floating around.

3) What I imagine as the most popular option, just don't write any tests.

Looking at what most of the requests are for, tells me most people are using Silverlight for Marketing websites to stream rich content. Business applications have yet to stake any dominance. What I am hoping that anyone reading this will if they feel my pain, just go to this link and vote. It seems that these polls really have an effect; DataTemplates appear to be part of SL 5 due to massive demand. I am hoping that Microsoft can focus on getting the underlying framework right before they go off and give us a 3D-multitouch-proximity aware API :)

http://dotnet.uservoice.com/forums/4325-silverlight-feature-suggestions/suggestions/313397-unit-testing-integrated-in-visual-studio-and-msbui?ref=title

Tuesday, November 2, 2010

Rx design guidelines

The Rx team have released a pdf specifiying their Design Guidelines to use when coding with the Reactive Extentions. The original post is here http://blogs.msdn.com/b/rxteam/archive/2010/10/28/rx-design-guidelines.aspx The PDF is here http://go.microsoft.com/fwlink/?LinkID=205219 Great stuff. Next; the FxCop rules for static analysis?

More guidance at IntroToRx.com in the Usage Guidelines appendix

Tuesday, October 26, 2010

Rx Part 8 - Testing Rx

STOP THE PRESS! This series has now been superseded by the online book www.IntroToRx.com. The new site/book offers far better explanations, samples and depth of content. I hope you enjoy!

Having reviewed the scheduling available to us in Rx and now that we are aware of Hot and Cold observables we have almost enough skills in our tool belt to start using Rx in anger. However many developers wouldn’t dream of starting coding without first being able to write tests to prove their code is in fact satisfying their requirements and providing them with that safety net against regression. Rx poses some interesting problems to our Test Driven community
  • Scheduling and therefore Threading are generally avoided in test scenarios as they can introduce race conditions which may lead to non-deterministic tests.
  • Tests should run as fast as possible.
  • Rx is a new technology/library so naturally as we master it, we will refactor our code. We want to use to tests to ensure our refactoring have not altered the internal behaviour of our code base.
So we want to test our code but don't want to introduce false-negatives, false-positives or non-deterministic tests. Also if we look at the Rx library there are plenty of methods that involve Scheduling so it is hard to ignore. This Linq query shows us that there are at least 32 extension methods that accept an IScheduler as a parameter
var query = from method in typeof (Observable).GetMethods()
        from parameter in method.GetParameters()
        where typeof (IScheduler).IsAssignableFrom(parameter.ParameterType)
        group method by method.Name into m
        orderby m.Key
        select m.Key;
            
query.Run(Console.WriteLine);
/* 
BufferWithTime, Catch, Concat, Delay, Empty, Generate, GenerateWithTime, Interval
Merge, ObserveOn, OnErrorResumeNext, Prune, Publish, Range, Repeat, Replay
Retry, Return, Sample, Start, StartWith, Subscribe, SubscribeOn, Take, Throttle
Throw, TimeInterval, Timeout, Timer, Timestamp, ToAsync, ToObservable
*/
There are some methods that we already will be familiar with such as ObserveOn and SubscribeOn. Then there are others that will optionally take an IScheduler in one of the method overloads. TDD/TestFirst coders will want to opt for the overload that takes the IScheduler so that we can have some control over scheduling in our tests.
In this example we create a stream that publishes values every second for 5 seconds.
var interval = Observable
    .Interval(TimeSpan.FromSeconds(1))
    .Take(5);
If we to write a test that ensured that we received 5 values and they were each 1 second apart it would take 5 seconds. That would be no good, I want hundreds if not thousands of tests to run in 5 seconds. A more common time related example that would need tests is setting a timeout.
var stream = Observable.Never<int>();

var exceptionThrown = false;
stream.Timeout(TimeSpan.FromMinutes(1))
    .Run(
        i => Console.WriteLine("This will never run."),
        ex => exceptionThrown = true);
Assert.IsTrue(exceptionThrown);
This test would take 1 minute to run. However if we did some test first code in this style, where we added the timeout after the test, running the test initially to fail (Red-Green-Refactor) would never complete. Hmmmm…..

TestScheduler

To our rescue comes the TestScheduler. This is a recent addition to the Rx library that takes the concept of a virtual scheduler to allow us emulate and control time. Cool.
The concept of a virtual scheduler can sort of be thought of a queue of actions to be executed that are each marked with the point in time they should be executed. Where actions are attempted to be scheduled at the same “time” the second collision will be scheduled or queued after the first action but marked as with the same point in “time” . When we use the TestScheduler we can either “drain the queue” by calling run which will execute all scheduled actions, or we can specify to run all tasks up to a point in time.
In this example we schedule a task on to the queue to be run immediately by using the simple overload. We then execute everything scheduled for the first tick (ie the first action).
var scheduler = new TestScheduler();
var wasExecuted = false;
scheduler.Schedule(() => wasExecuted = true);
Assert.IsFalse(wasExecuted);
scheduler.RunTo(1);         //execute 1 tick of queued actions
Assert.IsTrue(wasExecuted);
The TestScheduler type is actually an implementation of the abstract VirtualScheduler<TAbsolute, TRelative> where the TestSchuduler specifies long for both TAbsolute and TRelative. The net result is that the TestScheduler interface looks something like this
public class TestScheduler
{
    public TestScheduler();

    // Methods
    public long FromTimeSpan(TimeSpan timeSpan);
    public long Increment(long absolute, long relative);
    public void Run();
    public void RunTo(long time);
    public IDisposable Schedule(Action action);
    public IDisposable Schedule(Action action, TimeSpan dueTime);
    public IDisposable Schedule(Action action, long dueTime);
    public void Sleep(long ticks);
    public DateTimeOffset ToDateTimeOffset(long absolute);

    // Properties
    public DateTimeOffset Now { get; }
    public long Ticks { get; }
}
We should already be familiar with what the Schedule methods would do, the other method of interest for this post are the Run() and RunTo(long) methods. Run() will just execute all the actions that have been scheduled. RunTo(long) will execute all the actions that have been scheduled up to the time specified by the long value which represents ticks in this case. Having a quick look into the implementations for Schedule, RunTo and Run give us the insight we need to really understand the virtual schedulers.
public IDisposable Schedule(Action action)
{
    return this.Schedule(action, TimeSpan.Zero);
}
public IDisposable Schedule(Action action, TimeSpan dueTime)
{
    return this.Schedule(action, this.FromTimeSpan(dueTime));
}
public IDisposable Schedule(Action action, TRelative dueTime)
{
    BooleanDisposable disposable = new BooleanDisposable();
    TAbsolute local = this.Increment(this.Ticks, dueTime);
    Action action2 = delegate {
        if (!disposable.IsDisposed)
        {
            action();
        }
    };
    ScheduledItem<TAbsolute> item = new ScheduledItem<TAbsolute>(action2, local);
    this.queue.Enqueue(item);
    return disposable;
}

public void RunTo(TAbsolute time)
{
    while ((this.queue.Count > 0) && (this.queue.Peek().DueTime.CompareTo(time) <= 0))
    {
        ScheduledItem<TAbsolute> item = this.queue.Dequeue();
        this.Ticks = item.DueTime;
        item.Action();
    }
}

public void Run()
{
    while (this.queue.Count > 0)
    {
        ScheduledItem<TAbsolute> item = this.queue.Dequeue();
        this.Ticks = item.DueTime;
        item.Action();
    }
}
Obviously this is the internal implementation and would be subject to change, however as the documentation currently is quite weak for the VirtualScheduler and TestScheduler I think this use of reflection is helpful. This example should clear up what happens when items are scheduled at the same time.
var scheduler = new TestScheduler();
long dueTime = 4L;
scheduler.Schedule(() => Console.WriteLine("1"), dueTime);
scheduler.Schedule(() => Console.WriteLine("2"), dueTime);
scheduler.Schedule(() => Console.WriteLine("3"), dueTime+1);
scheduler.Schedule(() => Console.WriteLine("4"), dueTime+1);
Console.WriteLine("RunTo(dueTime)");
scheduler.RunTo(dueTime); 
Console.WriteLine("Run()");
scheduler.Run();
/* Output:
RunTo(dueTime)
1
2
Run()
3
4
*/

Testing Rx code

Now that we have learnt a little bit about the TestScheduler, lets look at how we could use it to get our 2 initial code snippets (Interval and TimeOut) to execute as fast as possible but still maintaining the semantics of time. In this example we generate our 5 values one second apart but pass in our TestScheduler to the Interval method.
[TestMethod]
public void Testing_with_test_scheduler()
{
    var scheduler = new TestScheduler();
    var interval = Observable
        .Interval(TimeSpan.FromSeconds(1), scheduler)
        .Take(5);

    bool isComplete = false;
    interval.Subscribe(Console.WriteLine, () => isComplete = true);

    scheduler.Run();

    Assert.IsTrue(isComplete); //Executes in less than 0.01s "on my machine"
}
While this is mildly interesting, what I think is more important is how we would test a real piece of code. Imagine if you will a Presenter that subscribes to a stream of prices. As prices are published it adds them to a ViewModel. Assuming this is a WPF or Silverlight implementation we take the liberty of enforcing that the subscription be done on the ThreadPool and the observing is executed on the Dispatcher.
public class MyPresenter
{
...
    public void Show(string symbol)
    {
        _myService.PriceStream(symbol)
                                    .SubscribeOn(Scheduler.ThreadPool)
                                    .ObserveOn(Scheduler.Dispatcher)
                                    .Subscribe(price=>_viewModel.Prices.Add(price));
    }
...
}
While this snippet of code may do what we want it to do, it will be hard to test as it is accessing the schedulers via static properties. To help my testing, I have created my own interface that exposes the same IScheduler implementations that the Scheduler type does.
public interface ISchedulerService
{
    IScheduler CurrentThread { get; }
    IScheduler Dispatcher { get; }
    IScheduler Immediate { get; }
    IScheduler NewThread { get; }
    IScheduler ThreadPool { get; }
    //IScheduler TaskPool { get; }
}
public sealed class SchedulerService : ISchedulerService
{
    public IScheduler CurrentThread { get { return Scheduler.CurrentThread; } }

    public IScheduler Dispatcher { get { return Scheduler.Dispatcher; } }

    public IScheduler Immediate { get { return Scheduler.Immediate; } }

    public IScheduler NewThread { get { return Scheduler.NewThread; } }

    public IScheduler ThreadPool { get { return Scheduler.ThreadPool; } }

    //public IScheduler TaskPool { get { return Scheduler.TaskPool; } }
}
This now allows me to inject an ISchedulerService which gives me a seam to help my testing. I can now write some tests for my Presenter.
[TestInitialize]
public void SetUp()
{
    _myServiceMock = new Mock<IMyService>();
    _viewModelMock = new Mock<IViewModel>();
    _schedulerService = new TestSchedulers();

    var prices = new ObservableCollection<decimal>();
    _viewModelMock.SetupGet(vm => vm.Prices).Returns(prices);
    _viewModelMock.SetupProperty(vm => vm.IsConnected);
}

[TestMethod]
public void Should_pass_symbol_to_MyService_PriceStream()
{
    var expected = "SomeSymbol";
    var priceStream = new Subject<decimal>();
    _myServiceMock.Setup(svc => svc.PriceStream(It.Is<string>(symbol=>symbol==expected))).Returns(priceStream);

    var sut = new MyPresenter(_myServiceMock.Object, _viewModelMock.Object, _schedulerService);
    sut.Show(expected);

    _myServiceMock.Verify();
}

[TestMethod]
public void Should_add_to_VM_Prices_when_MyService_publishes_price()
{
    decimal expected = 1.23m;
    var priceStream = new Subject<decimal>();
    _myServiceMock.Setup(svc => svc.PriceStream(It.IsAny<string>())).Returns(priceStream);

    var sut = new MyPresenter(_myServiceMock.Object, _viewModelMock.Object, _schedulerService);
    sut.Show("SomeSymbol");
    _schedulerService.ThreadPool.Schedule(() => priceStream.OnNext(expected));  //Schedule the OnNext
    _schedulerService.ThreadPool.RunTo(1);  //Execute the OnNext action
    _schedulerService.Dispatcher.RunTo(1);  //Execute the OnNext Handler (ie adding to the Prices collection)

    Assert.AreEqual(1, _viewModelMock.Object.Prices.Count);
    Assert.AreEqual(expected, _viewModelMock.Object.Prices.First());
}
These two tests show first a simple expectation that a string value passed to my Show(string) method will be passed to the underlying service. This is not at all relevant to Rx. The next test shows the usage of my implementation of the ISchedulerService specific for testing. It exposes all of the IScheduler properties as instances of TestSchedulers. This now allows me to inject TestSchedulers for testing which in-turn allows me to control the rate at which things are scheduled.
For those of you new to Moq, some of the syntax my be a little bit confusing. Where you see a Setup(..) or SetupGet(..) method call there is just a little bit of Expression magic that tells my mocks to return correct thing when called. The .Object property hanging off my mocks are the dynamically generated implementations of the interfaces. This next test I hope you find the most interesting. Here we really get the full value out of our TestScheduler, by testing timeouts.
[TestMethod]
public void Should_timeout_if_no_prices_for_10_seconds()
{
    var timeoutPeriod = TimeSpan.FromSeconds(10);
    var priceStream = Observable.Never<decimal>();
    _myServiceMock.Setup(svc => svc.PriceStream(It.IsAny<string>())).Returns(priceStream);

    var sut = new MyPresenter(_myServiceMock.Object, _viewModelMock.Object, _schedulerService);
    sut.Show("SomeSymbol");

    _schedulerService.ThreadPool.RunTo(timeoutPeriod.Ticks - 1);
    Assert.IsTrue(_viewModelMock.Object.IsConnected);

    _schedulerService.ThreadPool.RunTo(timeoutPeriod.Ticks);
    Assert.IsFalse(_viewModelMock.Object.IsConnected);
}
The key points to note are:
  1. We return an Observable.Never for our price stream so that no prices are ever pushed and no OnComplete is published either.
  2. The _schedulerService is a test fake that returns TestSchedulers for all of it’s schedulers
  3. We run the ThreadPool TestScheduler up until 1 tick away from our timeout period and ensure that we have not timed out
  4. We run the ThreadPool TestScheduler up to our timeout period and then ensure that we have timed out.
  5. The test is sub second even though we are testing for a 10 second timeout!
Generally I only want to have one assertion in each of my tests, but for this example I think it elegantly tests that the Timeout is for 10seconds and not longer or shorter. If I was to remove the first assertion my test would only prove that the timeout was no greater than 10 seconds but could reasonably be set to say 3 seconds. The implementation for this simple presenter is as follows
public class MyPresenter
{
    private readonly IMyService _myService;
    private readonly IViewModel _viewModel;
    private readonly ISchedulerService _schedulerService;

    public MyPresenter(IMyService myService, IViewModel viewModel, ISchedulerService schedulerService)
    {
        _myService = myService;
        _schedulerService = schedulerService;
        _viewModel = viewModel;
    }

    public void Show(string symbol)
    {
        _myService.PriceStream(symbol)
                    .SubscribeOn(_schedulerService.ThreadPool)
                    .ObserveOn(_schedulerService.Dispatcher)
                    .Timeout(TimeSpan.FromSeconds(10), _schedulerService.ThreadPool)
                    .Subscribe(OnPriceUpdate, ex =>
                                                    {
                                                        if (ex is TimeoutException)
                                                            _viewModel.IsConnected = false;
                                                    });
        _viewModel.IsConnected = true;
    }

    private void OnPriceUpdate(decimal price)
    {
        _viewModel.Prices.Add(price);
    }
}
I hope this post on testing helps you bring you skills that you have been developing in Rx to a level where you may be comfortable considering them for production use.
For your reference here are the two test versions of the ISchedulerService I find useful. One will just schedule everything with the ImmediateScheduler and the other uses the TestSchedulers for fine grained control.
public sealed class TestSchedulers : ISchedulerService
{
    private readonly TestScheduler _currentThread = new TestScheduler();
    private readonly TestScheduler _dispatcher = new TestScheduler();
    private readonly TestScheduler _immediate = new TestScheduler();
    private readonly TestScheduler _newThread = new TestScheduler();
    private readonly TestScheduler _threadPool = new TestScheduler();

    #region Implementation of ISchedulerService
    IScheduler ISchedulerService.CurrentThread { get { return _currentThread; } }

    IScheduler ISchedulerService.Dispatcher { get { return _dispatcher; } }

    IScheduler ISchedulerService.Immediate { get { return _immediate; } }

    IScheduler ISchedulerService.NewThread { get { return _newThread; } }

    IScheduler ISchedulerService.ThreadPool { get { return _threadPool; } }
    #endregion

    public TestScheduler CurrentThread { get { return _currentThread; } }

    public TestScheduler Dispatcher { get { return _dispatcher; } }

    public TestScheduler Immediate { get { return _immediate; } }

    public TestScheduler NewThread { get { return _newThread; } }

    public TestScheduler ThreadPool { get { return _threadPool; } }
}

public sealed class ImmediateSchedulers : ISchedulerService
{
    public IScheduler CurrentThread { get { return Scheduler.Immediate; } }

    public IScheduler Dispatcher { get { return Scheduler.Immediate; } }

    public IScheduler Immediate { get { return Scheduler.Immediate; } }

    public IScheduler NewThread { get { return Scheduler.Immediate; } }

    public IScheduler ThreadPool { get { return Scheduler.Immediate; } }
}
The full source code is now available either via svn at http://code.google.com/p/rx-samples/source/checkout or as a zip file.
Back to the contents page for Reactive Extensions for .NET Introduction
Back to the previous post; Rx Part 7 – Hot and Cold Observables
Forward to next post; Part 9 – Join, Window, Buffer and Group Join
Technorati Tags: ,,

Friday, August 14, 2009

How not to write an Interface in C# 3.0

While working with the IUnityContainer interface from Microsoft's Patterns and Practices team, I decided that it was well worth a post on how not to design interfaces. Recent discussion amongst Rhys and Colin (here as well) have been interesting but I imagine most would agree that both arguments are probably fighting over which polish to use on their code. If these are the big battles you have to face at work then I am jealous.

Introducing IUnityContainer ....
42 members are defined on this interface. Fail.
With much restraint I wont go on about it and flame the guys who write it. Most people know of my disdain for the P&P team at Microsoft. So what tips do I give to make this a usable again? Lets break down the solution into a few tips:

  • Extension methods
  • Cohesion
  • Intention revealing interfaces

How do each of these help us. Let look at some code I was writing pre-extension methods. It was a validation interface that had 2 methods, Validate and Demand

public interface IValidator<T>
{
  IEnumerable<ValidationErrors> Validate(T item);

  ///<exception cref="ValidationException">Thrown when the item is not valid</exception>
  void Demand(T item);
}

The problem with this interface is that all implementations of the interface would/could implement Demand the same way; Make a call to Validate(T item) and if any ValidationErrors came back throw an exception with the validation failures. Time ticks by and I realise the now that I have extension methods in C# 3.0 (.NET 3.5). I only need to have the Validate method on my interface now and provide the Demand implementation as an extension method. The code now becomes:

public interface IValidation<T>
{
  /// <summary>
  /// Validates the specified item, returning a collection of validation errors.
  /// </summary>
  /// <param name="item">The item to validate.</param>
  /// <returns>Returns a <see cref="T:ArtemisWest.ValidationErrorCollection"/> that is empty for a valid item, 
  /// or contains the validation errors if the item is not in a valid state.</returns>
  /// <remarks>
  /// Implementations of <see cref="T:ArtemisWest.IValidation`1"/> should never return null from the validate method.
  /// If no validation errors occured then return an empty collection.
  /// </remarks>
  IEnumerable<ValidationErrors> Validate(T item);
}
public static class ValidatorExtensions
{
  /// <summary>
  /// Raises a <see cref="T:ArtemisWest.ValidationException"/> if the <paramref name="item"/> is not valid.
  /// </summary>
  /// <typeparam name="T">The type that the instance of <see cref="T:IValidation`1"/> targets.</typeparam>
  /// <param name="validator">The validator.</param>
  /// <param name="item">The item to be validated.</param>
  /// <exception cref="T:ArtemisWest.ValidationException">
  /// Thrown when validating the <paramref name="item"/> returns any errors. Only the first 
  /// validation error is raised with the exception. Any validation errors that are marked as
  /// warnings are ignored.
  /// </exception>
  public static void Demand<T>(this IValidation<T> validator, T item)
  {
    foreach (var error in validator.Validate(item))
    {
      if (!error.IsWarning)
      {
        throw new ValidationException(error);
      }
    }
  }
}

Then end result is that the API feels the same as I have access to both methods, but the cost of implementing my interface is reduced to just its core concern.
So, extension methods is one trick we have in the bag. Next cohesion.

The recent discussion between Rhys and Colin is "how many members belong on an interface?" I think both will agree that answer is not 42. Juval Lowy made a great presentation at TechEd2008 on Interfaces and that we should be aiming for 3-7 members per interface. This provides us with a level of cohesion and a low level of coupling. Lets look at some of the members on the IUnityContainer Interface.

  • 8 overloads of RegisterInstance
  • 16 overloads of RegisterType
  • Add/Remove "Extensions" methods
  • 4 overloads of BuildUp
  • 2 overloads of ConfigureContainer
  • CreateChildContainer
  • 4 overloads of Resolve
  • 2 overloads of ResolveAll
  • A TearDown method
  • and a Parent property
Whew! Well how can we tame this beast? When I look at this interface I see certain groups that look like they are related in usage. They would be the
  1. Register and Resolve functionality
  2. And and Remove extensions functionality
  3. Build up and teardown functionality
  4. Container hierarchy functionality (Parent and CreateChildContainer)
  5. Container configuration


Interestingly on our current project we only use the Register/Resolve functionality.
These five groups to me have some level of cohesion. That to me then makes them a candidate for there own interfaces. The big giveaway being that I use unity quite successfully but never use 4/5 of the functionality defined on this interface. So our 2nd tip would be to split out these groups on functionality into there own interfaces.
But what do we name the new interfaces? This is our 3rd tip:

Intention revealing interfaces.

Looking at my list I would imagine some useful names could be:

  1. IContainer
  2. IExtensionContainer
  3. ILifecycleContainer
  4. INestedContainer
  5. IConfigurableContainer
To be honest, I have put little thought into these names. Normally I would put a LOT of effort into getting the naming right, but I don't work on the P&P team so these changes will never be done so why waste my time? Edit: My laziness here really does take the wind our of the sails of this argument. Sorry.
Ok so how can we bring this all together? My proposal would be to have 6 interfaces
  1. IContainer
  2. IExtensionContainer
  3. ILifecycleContainer
  4. INestedContainer
  5. IConfigurableContainer
  6. IUnityContainer : IContainer, IExtensionContainer, ILifecycleContainer, INestedContainer, IConfigurableContainer


Next I would create some extension methods to deal with the silly amount of duplication the multiple overloads incur. Looking at the implementation in UnityContainerBase I would think that all of these methods could be candidates for extension methods

public abstract class UnityContainerBase : IUnityContainer, IDisposable
{
  //prior code removed for brevity
  public IUnityContainer RegisterInstance<TInterface>(TInterface instance)
  {
    return this.RegisterInstance(typeof(TInterface), null, instance, new ContainerControlledLifetimeManager());
  }
  public IUnityContainer RegisterInstance<TInterface>(string name, TInterface instance)
  {
    return this.RegisterInstance(typeof(TInterface), name, instance, new ContainerControlledLifetimeManager());
  }
  public IUnityContainer RegisterInstance<TInterface>(TInterface instance, LifetimeManager lifetimeManager)
  {
    return this.RegisterInstance(typeof(TInterface), null, instance, lifetimeManager);
  }
  public IUnityContainer RegisterInstance(Type t, object instance)
  {
    return this.RegisterInstance(t, null, instance, new ContainerControlledLifetimeManager());
  }
  public IUnityContainer RegisterInstance<TInterface>(string name, TInterface instance, LifetimeManager lifetimeManager)
  {
    return this.RegisterInstance(typeof(TInterface), name, instance, lifetimeManager);
  }
  public IUnityContainer RegisterInstance(Type t, object instance, LifetimeManager lifetimeManager)
  {
    return this.RegisterInstance(t, null, instance, lifetimeManager);
  }
  public IUnityContainer RegisterInstance(Type t, string name, object instance)
  {
    return this.RegisterInstance(t, name, instance, new ContainerControlledLifetimeManager());
  }
  //Remaining code removed for brevity
}

all of these methods just delegate to the one method overload left as abstract

public abstract IUnityContainer RegisterInstance(
    Type t, 
    string name, 
    object instance, 
    LifetimeManager lifetime);

The obvious thing to do here is to just make all of these extension methods in the same namespace as the interfaces

public static class IContainerExtensions
{
  public IContainer RegisterInstance<TInterface>(this IContainer container, TInterface instance)
  {
    return container.RegisterInstance(typeof(TInterface), null, instance, new ContainerControlledLifetimeManager());
  }
  public IContainer RegisterInstance<TInterface>(this IContainer container, string name, TInterface instance)
  {
    return container.RegisterInstance(typeof(TInterface), name, instance, new ContainerControlledLifetimeManager());
  }
  public IContainer RegisterInstance<TInterface>(this IContainer container, TInterface instance, LifetimeManager lifetimeManager)
  {
    return container.RegisterInstance(typeof(TInterface), null, instance, lifetimeManager);
  }
  public IContainer RegisterInstance(this IContainer container, Type t, object instance)
  {
    return container.RegisterInstance(t, null, instance, new ContainerControlledLifetimeManager());
  }
  public IContainer RegisterInstance<TInterface>(this IContainer container, string name, TInterface instance, LifetimeManager lifetimeManager)
  {
    return container.RegisterInstance(typeof(TInterface), name, instance, lifetimeManager);
  }
  public IContainer RegisterInstance(this IContainer container, Type t, object instance, LifetimeManager lifetimeManager)
  {
    return container.RegisterInstance(t, null, instance, lifetimeManager);
  }
  public IContainer RegisterInstance(this IContainer container, Type t, string name, object instance)
  {
    return container.RegisterInstance(t, name, instance, new ContainerControlledLifetimeManager());
  }
}

This now reduces our IContainer Interface to just the one method

public interface IContainer
{
  IContainer RegisterInstance(Type t, string name, object instance, LifetimeManager lifetime);
}


One thing to note here is that we have broken the contract because we now return IContainer not IUnityContainer. We will come back to this problem later.
If we then follow suit on the other interfaces we have created, we will have 6 interfaces that look like this:

public interface IContainer
{
  IContainer RegisterType(Type from, Type to, string name, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers);
  IContainer RegisterInstance(Type t, string name, object instance, LifetimeManager lifetime);
  object Resolve(Type t, string name);
  IEnumerable<object> ResolveAll(Type t);
}
public interface IExtensionContainer
{
  IExtensionContainer AddExtension(UnityContainerExtension extension);
  IExtensionContainer RemoveAllExtensions();
}
public interface ILifecycleContainer
{
  object BuildUp(Type t, object existing, string name);
  void Teardown(object o);
}
public interface INestedContainer
{
  INestedContainer CreateChildContainer();
  INestedContainer Parent { get; }
}
public interface IConfigurableContainer
{
  object Configure(Type configurationInterface);
}
public interface IUnityContainer : IDisposable, IContainer, IExtensionContainer, ILifecycleContainer, INestedContainer, IConfigurableContainer
{}

So now we have some much more managable interfaces to work with. However, we have broken the feature that it had before of returning IUnityContainer from each method. You may ask why would you return the instance when clearly you already have the instance? By doing so you can create a fluent interface. See my other post on Fluent interfaces and DSLs for more information.
Now that we have removed all the noise from the interfaces we actually have a reasonable number of members to work with. This makes me think that maybe we can refactor back to a single interface? Lets have a look:

public interface IUnityContainer : IDisposable
{
  IUnityContainer RegisterType(Type from, Type to, string name, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers);
  IUnityContainer RegisterInstance(Type t, string name, object instance, LifetimeManager lifetime);
  object Resolve(Type t, string name);
  IEnumerable<object> ResolveAll(Type t);
  IUnityContainer AddExtension(UnityContainerExtension extension);
  IUnityContainer RemoveAllExtensions();
  object BuildUp(Type t, object existing, string name);
  void Teardown(object o);
  IUnityContainer Parent { get; }
  IUnityContainer CreateChildContainer();
  object Configure(Type configurationInterface);
}

Well that is 13 members, which is above my happy limit of 10 and nearly double my ideal limit of 7, however...I think this would be a vast improvement on the silly interface that currently exists with its 42 members.
Just for fun here are the Extension methods that would complete the interface to bring it back to feature complete.

public static class IUnityContainerExtentions
{
  public static IUnityContainer AddNewExtension<TExtension>(this IUnityContainer container) where TExtension : UnityContainerExtension, new()
  {
    return container.AddExtension(Activator.CreateInstance<TExtension>());
  }
  public static T BuildUp<T>(this IUnityContainer container, T existing)
  {
    return (T)container.BuildUp(typeof(T), existing, null);
  }
  public static object BuildUp(this IUnityContainer container, Type t, object existing)
  {
    return container.BuildUp(t, existing, null);
  }
  public static T BuildUp<T>(this IUnityContainer container, T existing, string name)
  {
    return (T)container.BuildUp(typeof(T), existing, name);
  }
  public static TConfigurator Configure<TConfigurator>(this IUnityContainer container) where TConfigurator : IUnityContainerExtensionConfigurator
  {
    return (TConfigurator)container.Configure(typeof(TConfigurator));
  }
  public static IUnityContainer RegisterInstance<TInterface>(this IUnityContainer container, TInterface instance)
  {
    return container.RegisterInstance(typeof(TInterface), null, instance, new ContainerControlledLifetimeManager());
  }
  public static IUnityContainer RegisterInstance<TInterface>(this IUnityContainer container, string name, TInterface instance)
  {
    return container.RegisterInstance(typeof(TInterface), name, instance, new ContainerControlledLifetimeManager());
  }
  public static IUnityContainer RegisterInstance<TInterface>(this IUnityContainer container, TInterface instance, LifetimeManager lifetimeManager)
  {
    return container.RegisterInstance(typeof(TInterface), null, instance, lifetimeManager);
  }
  public static IUnityContainer RegisterInstance(this IUnityContainer container, Type t, object instance)
  {
    return container.RegisterInstance(t, null, instance, new ContainerControlledLifetimeManager());
  }
  public static IUnityContainer RegisterInstance<TInterface>(this IUnityContainer container, string name, TInterface instance, LifetimeManager lifetimeManager)
  {
    return container.RegisterInstance(typeof(TInterface), name, instance, lifetimeManager);
  }
  public static IUnityContainer RegisterInstance(this IUnityContainer container, Type t, object instance, LifetimeManager lifetimeManager)
  {
    return container.RegisterInstance(t, null, instance, lifetimeManager);
  }
  public static IUnityContainer RegisterInstance(this IUnityContainer container, Type t, string name, object instance)
  {
    return container.RegisterInstance(t, name, instance, new ContainerControlledLifetimeManager());
  }
  public static IUnityContainer RegisterType<T>(this IUnityContainer container, params InjectionMember[] injectionMembers)
  {
    return container.RegisterType(typeof(T), null, null, null, injectionMembers);
  }
  public static IUnityContainer RegisterType<TFrom, TTo>(this IUnityContainer container, params InjectionMember[] injectionMembers) where TTo : TFrom
  {
    return container.RegisterType(typeof(TFrom), typeof(TTo), null, null, injectionMembers);
  }
  public static IUnityContainer RegisterType<T>(this IUnityContainer container, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers)
  {
    return container.RegisterType(typeof(T), null, null, lifetimeManager, injectionMembers);
  }
  public static IUnityContainer RegisterType<TFrom, TTo>(this IUnityContainer container, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers) where TTo : TFrom
  {
    return container.RegisterType(typeof(TFrom), typeof(TTo), null, lifetimeManager, injectionMembers);
  }
  public static IUnityContainer RegisterType<T>(this IUnityContainer container, string name, params InjectionMember[] injectionMembers)
  {
    return container.RegisterType(typeof(T), null, name, null, injectionMembers);
  }
  public static IUnityContainer RegisterType<TFrom, TTo>(this IUnityContainer container, string name, params InjectionMember[] injectionMembers) where TTo : TFrom
  {
    return container.RegisterType(typeof(TFrom), typeof(TTo), name, null, injectionMembers);
  }
  public static IUnityContainer RegisterType(this IUnityContainer container, Type t, params InjectionMember[] injectionMembers)
  {
    return container.RegisterType(t, null, null, null, injectionMembers);
  }
  public static IUnityContainer RegisterType<T>(this IUnityContainer container, string name, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers)
  {
    return container.RegisterType(typeof(T), null, name, lifetimeManager, injectionMembers);
  }
  public static IUnityContainer RegisterType<TFrom, TTo>(this IUnityContainer container, string name, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers) where TTo : TFrom
  {
    return container.RegisterType(typeof(TFrom), typeof(TTo), name, lifetimeManager, injectionMembers);
  }
  public static IUnityContainer RegisterType(this IUnityContainer container, Type t, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers)
  {
    return container.RegisterType(t, null, null, lifetimeManager, injectionMembers);
  }
  public static IUnityContainer RegisterType(this IUnityContainer container, Type t, string name, params InjectionMember[] injectionMembers)
  {
    return container.RegisterType(t, null, name, null, injectionMembers);
  }
  public static IUnityContainer RegisterType(this IUnityContainer container, Type from, Type to, params InjectionMember[] injectionMembers)
  {
    container.RegisterType(from, to, null, null, injectionMembers);
    return container;
  }
  public static IUnityContainer RegisterType(this IUnityContainer container, Type t, string name, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers)
  {
    return container.RegisterType(t, null, name, lifetimeManager, injectionMembers);
  }
  public static IUnityContainer RegisterType(this IUnityContainer container, Type from, Type to, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers)
  {
    return container.RegisterType(from, to, null, lifetimeManager, injectionMembers);
  }
  public static IUnityContainer RegisterType(this IUnityContainer container, Type from, Type to, string name, params InjectionMember[] injectionMembers)
  {
    return container.RegisterType(from, to, name, null, injectionMembers);
  }
  public static T Resolve<T>(this IUnityContainer container)
  {
    return (T)container.Resolve(typeof(T));
  }
  public static T Resolve<T>(this IUnityContainer container, string name)
  {
    return (T)container.Resolve(typeof(T), name);
  }
  public static object Resolve(this IUnityContainer container, Type t)
  {
    return container.Resolve(t, null);
  }
  public static IEnumerable<T> ResolveAll<T>(this IUnityContainer container)
  {
    //return new <ResolveAll>d__0<T>(-2) { <>4__this = this };
    //This implementation requires more effort than I am willing to give (6hours till my holiday!)
  }
}

Bloody hell. Imagine trying to implement that on every class that implemented the interface!

While I know this entire post is academic as we cant change Unity, but I hope that sews some seeds for other developers so that when they design their next interface it wont have silly overloads that just make the consuming developer's job harder than it ought to be.

Sunday, July 26, 2009

Code inspections

Code Inspections (or Code Reviews or Peer Reviews) is a technique a team can implement to improve the quality of the software they deliver. It is initially a bit of a cultural shift for many teams but can produce benefits not just in software but if done well also in team morale.

Goals

The goals for performing a Code Inspection are the following:

  1. To cross pollinate Domain knowledge
  2. To cross pollinate technical knowledge
  3. To maintain a consistent team process
  4. To identify any obvious errors

A code inspection should be an enjoyable experience where two developers are able to bond and cooperatively improve the quality of software delivered. The exercise should not feel like a witch-hunt or a boasting exercise for one developer to flex their ability over another. Initially the exercise may feel somewhat uncomfortable for the team, this is usually due to many different styles and tolerances the each member of the team brings to the collective skill set. In these scenarios it is better to agree amongst the team the level of detail that is in the remit of the inspections.

Introducing a team to Code Inspections

It is probably best if a team ease into code inspections, for it is better to be doing some code inspections and forgiving style differences than it is to be too aggressive in initial code inspections and cause tension in the team which sees the exercise become avoided. While this may be difficult to accept initially, one must remember that increasing the quality of today's code is of less importance than spreading the knowledge through out the team and maintain a quality team spirit. To know what to target in initial inspections, the team should conduct a brief team meeting and pick a handful of areas that the team agree needs work. This way we can get the biggest gain without creating to much friction.

Tools

The "what" to look for is of less importance than the process itself but the process can be aided quite nicely by the use of automated tools. In the .NET community the tools I would recommend are FxCop (or the Visual Studio Code Analysis), Simian (identifies duplicate code) and NCover (for test coverage).
To ease into code inspections I think it is best to set the bar quite low. For FxCop just enable the following Rules
  • Design
  • Naming
  • Performance
  • Security
  • Usage
While some teams may have their own style that might conflict with some of the styles in FxCop, ultimately this is a community standard now. As developers move between jobs it is great to eliminate this kind of induction friction when all .NET developers can be "singing from the same hymn sheet".
Simian provides the ability to set a tolerance of duplication to 6 lines of duplicate code. This should be fine.
Assuming the team is TDD or at least writing tests the the bar for test coverage could start at something like 80%. While I understand that chasing 100% test coverage can be frowned upon, it is nice to know that code was writing for a reason and does what it was intended on doing.

Setting expectations

Once the initial team meeting has been completed a set of expectations should be drawn up. This will be the bench mark for code inspections. The benchmark should be set at a level that everyone in the team agrees that they are happy for their code to be "rejected" if it does not meet the expectations. As you can imagine this in itself will lower the bar quite considerably as developers know that there are circumstances beyond their control which may prohibit them meeting the expectations that they would like to meet. Project deadlines, new technologies and processes can affect a developers ability to meet these goals. Remember the goals for code inspection is not perfect code, it is to improve the ability of the team to create better code, and continually get better at creating better code.
An example for an initial code inspection check list may look like this for a C# 3.0 project:
  1. Exceptions that are explicitly thrown from a public or protected members must be documented with XML comments.
  2. FxCop Naming and Performance rules must be met.
  3. Classes should be testable.
While this is a very small list of things to consider, just setting these expectations combined with knowledge sharing that the team would perform would see a dramatic improvement on any team not already doing code inspections.
Once the team is comfortable with the process and culture of code inspections, have another meeting to raise the bar a little bit. Again, refrain from being too eager. All team member should feel comfortable with the expectations being set.

Performing Code Inspections

Be fore we cover the actual actions of what to do when doing a code inspection just quickly remeber the goals of a code inspection
  1. To cross pollinate Domain knowledge
  2. To cross pollinate technical knowledge
  3. To maintain a consistent team process
  4. To identify any obvious errors

The inspection is not to create perfect code.
Steps to perform a code inspection (the first few step are as a courtesy to the reviewer):
  1. First perform a quick review yourself.
  2. Indicate to the person you would like to review your code that you will want some of their time in a few moments. It is important that this person not be the same few people. Have a method where you cycle through the team (clock wise round the office, youngest to oldest etc). Generally you will want to do most of your reviews with your immediate team, but if you have a larger development team/department that can be involved in the inspection be sure to include them too. *
  3. Perform any automated analysis (FxCop, Simian, Unit tests etc...)
  4. Open all the necessary documents ready to go, and consider the logical path to show the reviewer through your code.
  5. Some prefer to review code on paper. This is very good for removing the desire to solve any problems with the code, it also allows you to easily make notes against the code and possibly perform the review in a quiet place. However, I also think that if done properly this creates a mass of wasted paper and ink.
  6. When both parties are ready, have the author describe the intent of the code to the reviewer. This should start with a 10 second overview of what set of code does. 
  7. Show the reviewer the output of any automated analysis (FxCop, Tests, Coverage etc.). Obviously the results of which should met the set expectations.
  8. If at the computer give the reviewer the mouse and keyboard. If using a print out have the print out in front of the reviewer.
  9. Describe what the code does, ideally from entry point to each of the leaves. Very good code will basically read like prose anyway so this would be very easy.
  10. The reviewer sets the pace of the review by scrolling or navigating to classes/methods as they understand the code.
  11. The reviewer prompts for "what-if" scenarios and bookmarks any concerns (Ctrl+k+k in Visual Studio). No attempt is made to "fix" any code by either party.
  12. As the reviewer bookmarks any code they should also take a note of why the bookmark was made (Unclear code, not meeting agreed expectations, possible error etc). This can be done on paper or if available a Code Inspection prompt sheet.
  13. The author generally will identify most of their mistakes as they describe the code to the reviewer.
  14. If the code has been flagged in any way that would fail to meet the set expectations -or- would result in probably faulty software; the Author requests the the Reviewer come back to verify their changes and approve the code.**
  15. Once the review(s) have come to a point where no probable faults exists in the code and the team expectations are met the code review is complete.


*In my experience my project teams have been generally in sizes of 3-8. In these scenarios I would try to have each member of the team review my code over the course of a week. Once I had engaged each team member I would next engage someone from another team. This allows for a knowledge transfers across teams too (like finding out each team is using different logging tools!).

**By having the Author suggest that they fix their code, this creates far less stress for the reviewer to feel as if they are judging the authors code. Essentially the reviewer is a soundboard and prompt for the author.

Hints and Tips

  • Ensure that the team is clear that both the Reviewer and the Author are there to find any defects. It is a cooperative approach.
  • Focus on finding defects and failed expectations, not fixing them.
  • The biggest benefit for performing Code Inspections is that it creates a culture where the team all get better technically and also domain knowledge is not isolated to pockets of people.
  • Inspect all new and modified code
  • For best results review often and review early. Not many developers write so much code that 4 reviews in a day would be necessary, but not may developers would write so little code that only 1 or no code reviews would be done in a day. An exmple of a set of code to review could be when an MVP screen is complete; review the Model, View, Presenter and tests. I imagine few developer produce more than 4 tested screens per day.
  • As you get better at code reviews they should become less and less formal and quicker to execute. Extreme Programming takes peer review to the next level by have 2 developer work together constantly in a fashion called Pair Programming.
  • Some teams keep track of their code reviews an can create statistics to analyse where current weaknesses are. If you see value in this than give it a go, but make sure it doesn't just create "busy work". I believe that if the inspections are done correctly the team will know that there is an area of need and should raise it appropriately.
  • Code reviews should be done at the time the code was completed. New code should not be started until the current code has been inspected and approved. Keeping thing fresh in your head is key to a good review. It also means that "unfinished"/un-inspected code doesn't just slip out into production.
  • Some source/version control systems allow you to attribute check ins. If this is the case then code that has passed a check in can be attributed ideally with the reviewer's details. This should then allow the team to analyse all code that has not been reviewed (ie. last check-in for a file was not attributed).

Tuesday, December 16, 2008

Web security

I recently helped a friend evaluate a web she had developed for her. The business is essentially a web based business, but she is not a web developer herself so had the development outsourced to a reputable local web design company. At first she was happy with the result them some things were going a miss and one of her business partners who fancied themselves as a hobbyist nerd found a security hole. This raised some concerns and then she approached them and they had it fixed up. When Alice* told me about this I was rather uncomfortable with the whole scenario. How is a hobbyist able to identify security holes in a web site?!

Alice sensed my concern (probably due to the swearing and head in hands), and asked if I was able to have a quick review of the site. The good news was they fixed up the SQL Injection vulnerability. Bad news was, that was about it. Now I am a professional developer working exclusively in the .NET space. I am aware of certain security issues and would place myself as a somewhat knowledgeable in areas of security, but by no means a Bruce Schneier or Keith Brown. It took me 4 hours with the use of notepad, skydrive and a browser (ie free and available software) to have cross-site scripting attacks up and running. I was able to fetch my own password from the cross site script and also hijack my session. Having done this I flicked Alice an email and let her know the bad news.

This made me think, are the web developers out there writing secure applications yet? In fact is anyone? The last place I worked, they commissioned a guy to write his own custom federated security model for an internal system. Yeah the words Federated and Internal seem to conflict eh. Well that system took only 90mins to hack open. Irony is that Windows Authentication would have been more secure and faster. The place I am currently working they have a recommendation to store username and password pairs in *.config files when trying to authenticate across multiple servers (Kerberos hop anyone?). This obviously is worrying.

So I thought I would throw together a refresher list of things you need to do to make your system, well not insecure.

  1. Don't store passwords. That's right you don't need them. Ideally you should be using an authentication system (OpenAuth, AD/LDAP etc.) but if you need to roll your own you only need a salted hash of the password to perform a comparison for authentication. Storing passwords is just asking for trouble. If you consider all of the systems the users have to log in to there is a very high chance that either their password is the same everywhere or they have a pattern for picking passwords. If I can hack your Database of passwords then not only can I hack you site, but there is a good chance that I can now hack these user's accounts in gmail, facebook, linkedin etc and even worse...bank accounts or work accounts.
  2. Sanitise your inputs, and even better don't ever "evaluate" user input as code this comic is a great parody of this. An easy target of this is SQL Injection attacks. SQL Injection attacks just make me shudder.
  3. As above, only use parameterised SQL. This doesn't favour SPs or dynamic SQL. I have seen vulnerable code in both, but neither needs to be vulnerable.
  4. In web development, never display foreign or user input to the screen with out encoding it. This is how session hijacking occurs. (basically repeating myself on evaluating user input)
  5. Use HTTPS for login screens or any screen that passes private/protected data.
  6. .Net developers should strong name critical assemblies. If you don't, some one can basically use reflector to recreate your assembly and replace your implementation with theirs. I have used this to replace the assembly that performed the login. My implementation was identical except that I logged passwords to a location that I could read later. Interestingly the compiled assembly was identical in size even though I had added code. Changing the file created/modified date to the original wasn't a hassle so no one would be any wiser.

Rocky H used to have a brilliant video on "Assembly Hijacking" which showcased a combination styles of attacks to basically take over a server. The link I have (and the ones Google finds) are dead links. I have emailed him to see if we can get it back on line. Required viewing.

I hope the information here helps someone. If you dont know what any of the jargon above means in detail then there is a great chance that you are writing insecure code. Just Google/Wiki the phrase you don't know, it will take you 10minutes to learn what it is and how to prevent it. Otherwise feel free to ask.

*Alice is not her real name

Wednesday, November 5, 2008

Fail fast

Big Col nails the philosophy of fail fast in his recent blog posts. His first post covers this philosophy at a system level and his second post discusses what you can do in your C#/VB.NET/Java code until fancy Eiffel constructs like spec# make there way in to our languages.

Friday, October 31, 2008

Framework Design Guidelines v2

Brad and Krys are just about to release v2 of their great book Framework Design Guidelines. Here is a link to their recent "pre-release" talk at PDC. I am a big fan of this subject area and find it relevant to all levels of developers. So if you havn't read the original book try to get this new version, or at least watch the video ;-)

Friday, October 3, 2008

Sneaky little WCF error

I was going to drop a post about a WCF issue that had the team scratching their head yesterday afternoon and this morning. However one of the guys on the team spotted this article.

Basically we didn’t validate properly before we threw some enums over the wire, but it would have been nice for WCF to give us an error besides “Connection was closed unexpectedly” which doesn't really lead you to think that a serialization issue is to blame.

Thursday, August 21, 2008

FxCop 1.36 is out

I’m a bit of a fan of FxCop. Obviously it is a great static code analysis tool, but it is also great tool for learning. The new version has some bug fixes and support for new language features like anonymous methods and lambda expressions.

See the release note from the Code analysis and Code Metrics blog.