Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

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

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?

like image 890
Scott Whitlock Avatar asked Dec 02 '22 04:12

Scott Whitlock


2 Answers

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.

like image 179
Brandon Avatar answered Dec 03 '22 17:12

Brandon


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)

like image 28
Lou Franco Avatar answered Dec 03 '22 18:12

Lou Franco