Having trouble with KISSing

Question

There's a bit of a weird piece in my API that I'm not too happy about, but I can't seem to see any other way of going about.

It involves a IFunctionalityFactory abstract factory:

public interface IFunctionalityFactory
{
    IFunctionality Create();
}

An IFunctionality interface:

public interface IFunctionality
{
    void Execute();
    string AuthId { get; }
    bool IsAuthorised { get; }
}

Sample implementation - I don't like that, because most functionalities will have pretty much exactly the same identical code, except for the type name... although I like how simple and straightforward this code has become:

public class SomeFunctionality : FunctionalityBase
{
    private readonly IView _view;

    public SomeFunctionality(bool canExecute, IView view) 
        : base(canExecute)
    {
        _view = view;
    }

    public override void Execute()
    {
        _view.ShowDialog();
    }
}

Here's the base class:

public abstract class FunctionalityBase : IFunctionality
{
    private bool _canExecute;

    protected FunctionalityBase(bool canExecute)
    {
        _canExecute = canExecute;
    }

    public virtual void Execute()
    {
        // Templated method.
        // Must override in all derived classes.
        throw new NotImplementedException();
    }

    void IFunctionality.Execute()
    {
        if (_canExecute)
        {
            Execute();
        }
        else
        {
            // client code should catch and gracefully handle this exception:
            throw new NotAuthorizedException(resx.Functionality_Execute_NotAuthorised);
        }
    }

    string IFunctionality.AuthId
    {
        get { return GetType().FullName; }
    }

    bool IFunctionality.IsAuthorised
    {
        get { return _canExecute; }
    }
}

Also involves a IFunctionalityAuthorisation interface:

public interface IFunctionalityAuthorisation
{
    bool IsAuthorised(string authId);
}

The idea is that there's a database table that associates a string (AuthId) with zero or more ActiveDirectory groups; if there's no entry for a given AuthId, then the functionality is authorised for everyone. If there's one or more entries, it's only authorised for the users in the listed groups.

Here's a sample implementation:

public class SomeFunctionalityFactory : IFunctionalityFactory
{
    private readonly IFunctionalityAuthorisation _auth;
    private readonly IView _view;
    private readonly ISomeFunctionalityViewModel _viewModel;

    public SomeFunctionalityFactory(IFunctionalityAuthorisation auth, 
                                    IView view, ISomeFunctionalityViewModel viewModel)
    {
        _auth = auth;
        _view = view;
        _viewModel = viewModel;
    }

    public IFunctionality Create()
    {
        var canExecute = _auth.IsAuthorised(typeof(SomeFunctionality).FullName);
        _view.DataContext = _viewModel;
        return new SomeFunctionality(canExecute, _view);
    }
}

If a functionality factory has a IFunctionalityAuthorisation in its constructor, it receives this implementation:

public class FunctionalityAuthorisation : IFunctionalityAuthorisation
{
    private readonly ICurrentUser _user;
    private readonly ISecurityModel _model;

    public FunctionalityAuthorisation(ICurrentUser user, ISecurityModel model)
    {
        _user = user;
        _model = model;
    }

    public bool IsAuthorised(string authId)
    {
        var authorizedGroups = _model.AuthorizedRoles(authId);
        var result = !authorizedGroups.Any() || _user.Groups.Any(authorizedGroups.Contains);

        return result;
    }
}

The main issue I'm having with this approach, is that since I'm going to be injecting dozens of IFunctionalityFactory into the constructors of many modules, so I need to tell my IoC container to differenciate between them, and I'm using Ninject so I'm using InjectAttribute:

_kernel.Bind<IFunctionalityFactory>()
       .To<SomeFunctionalityFactory>()
       .WhenTargetHas<SomeAttribute>(); // SomeAttribute : InjectAttribute

_kernel.Bind<IFunctionalityFactory>()
       .To<AnotherFunctionalityFactory>()
       .WhenTargetHas<AnotherAttribute>(); // AnotherAttribute : InjectAttribute

And so on and so forth. If I have 200 functionalities to implement, I'm going to have 200 kernel bindings, and I hate that. Especially when the rest of the assembly's dependencies are configured like this:

_kernel.Bind(t => t.From(_businessLayerAssembly)
                   .SelectAllClasses()
                   .BindDefaultInterface());

Another issue I'm having with this approach (perhaps the main one), is that a functionality gets fully resolved, View, ViewModel, Model and all, even if it's never called, even if the user isn't even authorised to execute it. With the 3 functionalities I have now it's not much of an issue, but when I get to 200 it's certainly going to bite me, and if someone takes over my code in 5 years it will be one of the first things they WTF over, I'm sure.

Might be worth saying that this project is a C# rewrite (.net 4.0 / VS2010 with EF 4.4) of a crippled VB6 app that needs to be done while keeping the VB6 code in production, as it is business-critical code; it will be rewritten in small chunks, one functionality at a time - hence the "functionality" approach. The VB6 code needs to know if a functionality can or can't be executed, because until the "switchboard" forms that hold all the functionalities are redone in XAML, I want them to use the COM-visible IFunctionality interface to determine whether or not to draw the associated button.

Lastly, this is what the code is like after major refactorings, which have greatly simplified lots of things. I believe there's still a few layers of useless complexity, but I need your help to see where it's at, at this point. I'm not asking for specific answers regarding how I could address some of the specific issues I'm having (e.g. the NinjectAttribute issue), rather to be told exactly what's wrong with this approach (I might "not like" something that's just the way it has to be), why, and what would be a better way.

Answer 1

First: Don't create a base class with a virtual method which simply throws. This moves problem detection from compile time (abstract member not implemented won't compile) to run time (forgotten to override or accidentally called base throws) - usually undesirable. Virtual method says "You can override me if you want but you don't have to".

Now your actual problem.

Let me see if I understand correctly what you are saying:

• You have N classes of type XyzFunctionality
• You also have N corresponding factory classes of type XyzFunctionalityFactory each responsible of creating the specific XyzFunctionality
• You also have N corresponding RequiresXyzFunctionality attributes which you use to inject the specific XyzFunctionalityFactory for IFunctionalityFactory members

Is that basically correct? If yes then this smells to me:

If you have a member in a class of type IFunctionalityFactory then this basically says: "I want a factory which can create IFunctionality objects for me and do not care what they actually do" while in fact this is big fat lie - why else would you decorate with an attribute saying "actually this requires a very specific functionality factory"?

If the application requires that object Foo gets injected with a specific functionality factory then express it by making the type SomeFunctionalityFactory rather then IFunctionalityFactory - no 200 attributes, no 200 kernel bindings.

Now you will probably say "What about unit testing"?

1. You could make a FunctionalityFactoryBase class similar to your FunctionalityBase class with a virtual Create however that's not a good idea for the same reasons why it's not a good idea for FunctionalityBase.
2. Make Create virtual in all factories. Easy to forget.
3. Create a marker interface IXyzFunctionalityFactory and use that instead. Smells.

I think I'd go with the first option. The factories are all very light and really only contain Create so it's unlikely the implementer will forget to overwrite it. Unit tests should catch inadvertent calls of the base class method.

I might have assumed totally incorrectly though so please correct me if I'm wrong in my assumptions.