Code Contract, inheritance and Liskov Principle

Question

I have in my code the concept of command :

public abstract class BaseCommand
{
    public BaseCommand() { this.CommandId = Guid.NewGuid(); this.State = CommandState.Ready; }
    public Guid CommandId { get; private set; }
    public CommandState State {get; private set; }
    protected abstract void OnExecute();
    public void Execute() {
         OnExecute();
         State = CommandState.Executed;
    }
}

And some concrete implementation like this one :

public class DeleteItemCommand
{
    public int ItemId {get; set;}
    protected override void OnExecute()
    {
        var if = AnyKindOfFactory.GetItemRepository();
        if.DeleteItem(ItemId);
    }
}    

Now I want to add some validation. The first thing I can do is add a if/throw check:

public class DeleteItemCommand
{
    public int ItemId {get; set;}
    protected override void Execute()
    {
        if(ItemId == default(int)) throw new VeryBadThingHappendException("ItemId is not set, cannot delete the void");
        var if = AnyKindOfFactory.GetItemRepository();
        if.DeleteItem(ItemId);
    }
}

Now, I'm trying to use Code Contracts, because I'm quite convinced of its usefulness to reduce bug risk. If I rewrote the method like this :

public class DeleteItemCommand
{
    public int ItemId {get; set;}
    public void Execute()
    {
        Contract.Requires<VeryBadThingHappendException>(ItemId != default(int));

        var if = AnyKindOfFactory.GetItemRepository();
        if.DeleteItem(ItemId);
    }
}

The method compiles, the check is done at run-time. However, I got warning :

warning CC1032: CodeContracts: Method 'MyProject.DeleteItemCommand.Execute' overrides 'MyProject.BaseCommand.Execute', thus cannot add Requires.

I understand this warning is issued because I'm breaking the Liskov Principle.

However, in my case, conditions are different from one concrete class to another. My BaseCommand class is actually defining some common attributes like CommandIdentifier, state, and other ultimate features I removed here to keep the question simple.

While I understand the concepts of this principle, I don't know what are the step I have to do to remove the warning properly (don't tell me about the #pragma warning remove).

1. Should I stop using code contracts in this case, where concrete implementations have specific requirements ?
2. Should I rewrite my commanding mechanism to have, for example, separation between the Command "arguments" and the Command "execution" ? (having one CommandeExecutor<TCommand> per concrete class). This would result in a lot more classes in my project.
3. Any other suggestion ?
4. [Edit] As suggested by adrianm, convert properties to readonly, add constructor parameters to populate the properties and check properties in the contructor

Answer 1

I don't think you can use code contracts in this case. I think, you cannot add preconditions in overridden methods, only invariants and postconditions are possible there. Your best bet might be be to refactor from inheritance to composition:

ICommandExecutor
{
    Execute(BaseCommand source);
}

public abstract class BaseCommand
{
    public ICommandExecutor Executor { get; private set; }
    public void Execute() 
    {
        this.Executor.Execute(this);
        State = CommandState.Executed;
    }
}

public class DeleteCommandExecutor : ICommandExecutor
{
    public void Execute(BaseCommand source)
    {
        Contract.Requires<VeryBadThingHappendException>(source.ItemId != default(int));
        var if = AnyKindOfFactory.GetItemRepository();
        if.DeleteItem(source.ItemId);
    }
}

Answer 2

You could change the code to perform the execution in a different method:

public class DeleteItemCommand: BaseCommand
{
    public int ItemId {get; set;}
    public override void Execute()
    {
        PrivateExecute(ItemId);
    }

    private void PrivateExecute(int itemId)
    {
        Contract.Requires<VeryBadThingHappendException>(itemId != default(int));

        var rep = AnyKindOfFactory.GetItemRepository();
        rep.DeleteItem(itemId);
    }
}

Answer 3

In this case the code contracts are directing your attention to a design error in your class structure, and you would be wise to heed this warning.

To see the problem, consider how the classes may be used.

protected void executeButton_Click(object sender, EventArgs args) 
{
    BaseCommand command = GetCurrenctlySelectedCommand();
    command.Execute();
}

If the variable command happens to hold an object of the type DeleteItemCommand, then that object has preconditions that must be met or an exception will be thrown. We would like to avoid this exception, so how can we verify that the precondition is met?

Unfortunately, there is no simple way to do this. We cannot reason about all the possible preconditions about every type of derived object that may inhabit that variable. In fact, that variable may contain a type of object that was not invented when this code was written. In fact, the type for that object may not even be in the accessibility domain of this method, if it was provided by a factory in another assembly.

Since there is no way to verify that preconditions are met for this object, we cannot ensure the correctness of this code. We can either conclude from this that code contracts are useless, or that the code is designed incorrectly.

I understand this warning is issued because I'm breaking the Liskov Principle.

So you admit it!

However, in my case, conditions are different from one concrete class to another. My BaseCommand class is actually defining some common attributes like CommandIdentifier, state, and other ultimate features I removed here to keep the question simple.

Your analysis itself suggests the proper alternative.

Instead of a BaseCommand abstract class, you should create a CommandAttributes concrete sealed class. Then include an instance of this class within all of your command objects.

By using composition rather than inheritence, each of your command classes get the functionality they need, and they can define whatever sorts of preconditions or postconditions they need. And any methods that use those classes can verify those preconditions are met and take advantage of the postconditions.

Answer 4

I would use the constructor to set all the correct values, instead of public property setters.

public class DeleteItemCommand
{
    public DeleteItemCommand(int itemId)
    {
        Contract.Requires<VeryBadThingHappendException>(itemId!= default(int));
        ItemId = itemId;
    }

    public int ItemId {get; private set;}
    public void Execute()
    {   
        var if = AnyKindOfFactory.GetItemRepository();
        if.DeleteItem(ItemId);
    }
}

Answer 5

Apparently, Code Contracts work best on Domain Objects. I am guessing your Command class is not a domain object.