Having trouble with KISSing

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 d

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"?

• 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.

• Make Create virtual in all factories. Easy to forget.

• 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.

StackExchange Code Review Q#32440, answer score: 7