Does this violate the Liskov substitution principle, and if so, what do I do about it?

Question

Use case: I'm using data templates to match a View to a ViewModel. Data templates work by inspecting the most derived type of the concrete type provided, and they don't look at what interfaces it provides, so I have to do this without interfaces.

I'm simplifying the example here and leaving out NotifyPropertyChanged, etc., but in the real world, a View is going to bind to the Text property. For simplicity, imagine that a View with a TextBlock would bind to a ReadOnlyText and a View with a TextBox would bind to WritableText.

class ReadOnlyText
{
    private string text = string.Empty;

    public string Text
    {
        get { return text; }
        set
        {
            OnTextSet(value);
        }
    }

    protected virtual void OnTextSet(string value)
    {
        throw new InvalidOperationException("Text is readonly.");
    }

    protected void SetText(string value)
    {
        text = value;
        // in reality we'd NotifyPropertyChanged in here
    }
}

class WritableText : ReadOnlyText
{
    protected override void OnTextSet(string value)
    {
        // call out to business logic here, validation, etc.
        SetText(value);
    }
}

By overriding OnTextSet and changing its behavior, am I violating the LSP? If so, what's a better way to do it?

Answer 1

Only if the specification of ReadOnlyText.OnTextSet() promises to throw.

Imagine code like this

public void F(ReadOnlyText t, string value)
{
    t->OnTextSet(value);
}

Does it make sense to you if this didn't throw? If not, then WritableText isn't substitutable.

It looks to me like WritableText should inherit from Text. If there's some shared code between ReadOnlyText and WritableText, put it in Text or in another class that they both inherit from (that inherits from Text)

Answer 2

I'd say depends on the contract.

If the contract for ReadOnlyText says "any attempt to set Text will throw an exception", you are certainly violating LSP.

If not, you still have an awkwardness in your code: a setter for a read only text.

That is an acceptable "denormalization" under given circumstances. I've not yet found a better way that doesn't rely on lots of code. a Clean interface would be in most cases:

IThingieReader
{
    string Text { get; }
    string Subtext { get; }
    // ...
}

IThingieWriter
{
    string Text { get; set; }
    string Subtext { get; set; }
    // ...
}

...and implementing the interfaces only when appropriate. However, that breaks down if you have to deal with instances where e.g. Text is writable and Subtext is not, and is a pain to do for many objects / properties.

Answer 3

Yes it does, it wouldn't if the protected override void OnTextSet(string value) also threw an exception that was of type "InvalidOperationException" or inherited from it.

You should have a base class Text and both ReadOnlyText and WritableText inheriting from it.

Answer 4

LSP states that a subclass should be substiutable for it's superclass (see stackoverflow question here). The question to ask yourself is, "Is writeable text a type of readonly text?" The answer is clearly "no", in fact these are mutually exclusive. So, yes, this code violates LSP. However, is writable text a type of readable text (not readonly text)? The answer is "yes". So I think the answer is to look at what it is you're trying to do in each case and possibly to change the abstraction a bit as follows:

class ReadableText
{
    private string text = string.Empty;
    public ReadableText(string value)
    {
        text = value;
    }

    public string Text
    {
        get { return text; }
    }
}          

class WriteableText : ReadableText
{
    public WriteableText(string value):base(value)
    {

    }

    public new string Text
    {
        set
        {
            OnTextSet(value);
        }
        get
        {
            return base.Text;
        }
    }
    public void SetText(string value)
    {
        Text = value;
        // in reality we'd NotifyPropertyChanged in here       
    }
    public void OnTextSet(string value)
    {
        // call out to business logic here, validation, etc.       
        SetText(value);
    }
}     

Just to be clear, we're hiding the Text property from the Readable class using the new keyword on the Text property in the Writeable class.
From http://msdn.microsoft.com/en-us/library/ms173152(VS.80).aspx: When the new keyword is used, the new class members are called instead of the base class members that have been replaced. Those base class members are called hidden members. Hidden class members can still be called if an instance of the derived class is cast to an instance of the base class.