Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Why do not call overridable methods in constructors?

This is an oversimplified example, but I have some real-life code that conceptually does the same thing (trying to validate values "set" accessor methods of derivative classes), and the Analyzer gives me "Do not call overridable methods in constructors." I'm trying to figure out if I should change my code, or ignore the warning. I can't think of any reason I should heed the warning.

public abstract class SimpleUrl
{
    protected string _url;
    public abstract string Url { get; set; }
    public SimpleUrl()
    { }
    public SimpleUrl(string Url)
    {
        this.Url = Url;
    }
}

public class HttpUrl : SimpleUrl
{
    public HttpUrl()
    { }
    public HttpUrl(string Url)
    {
        this.Url = Url;
    }
    public override string Url
    {
        get
        {
            return this._url;
        }
        set
        {
            if (value.StartsWith("http://"))
                this._url = value;
            else
                throw new ArgumentException();
        }
    }
}
like image 869
Edward Ned Harvey Avatar asked Jan 01 '14 00:01

Edward Ned Harvey


2 Answers

As T.S. said (thank you T.S.), the base constructor will always be called, when instantiating the derived type. If you do as I did in the question, where the derivative constructor does not explicitly specify which base constructor to use, then the parameterless base constructor is used.

public HttpUrl()           // : base() is implied.

And

public HttpUrl(string Url) // : base() is still implied.  
                           // Parameterless. Not :base(Url)

So the complete answer to this question is: The abstract or virtual methods declared in the base class are only overridable in the base class, if you sealed the derivative class. So to make clean code, you don't run this line in the base constructor:

this.Url = Url;   // Don't do this in the base constructor

You instead, should "sealed" the derivative class (or method). As follows:

public abstract class SimpleUrl
{
    protected string _url;
    public abstract string Url { get; set; }
    // Optionally declare base constructors that do *not* call Url.set
    // Not sure why these constructors are optional in the base, but
    // required in the derivative classes.  But they are.
    public SimpleUrl()
    { }
}

public sealed class HttpUrl : SimpleUrl
{
    public HttpUrl()   // Not sure why this is required, but it is.
    { }
    public HttpUrl(string Url)
    {
        // Since HttpUrl is sealed, the Url set accessor is no longer
        // overridable, which makes the following line safe.
        this.Url = Url;
    }
    public override string Url
    {
        get
        {
            return this._url;
        }
        set
        {
            if (value.StartsWith("http://"))
                this._url = value;
            else
                throw new ArgumentException();
        }
    }
}

Alternatively, you don't need to seal the entire derivative class, if you just seal the overridable method (making it no longer overridable by any further derivative classes)

public class HttpUrl : SimpleUrl
{
    // ...
    public override sealed string Url
    // ...
like image 185
Edward Ned Harvey Avatar answered Oct 16 '22 11:10

Edward Ned Harvey


The answer really is, this can lead to unexpected behavior.

http://msdn.microsoft.com/en-us/library/ms182331.aspx

Something you missed in your code:

public class HttpUrl : SimpleUrl
{
    public HttpUrl()**:base()**
    { }
    public HttpUrl(string Url)**:base(Url)**
    {
        this.Url = Url;
    }
.........
}

Do you see now? You can't NOT have constructor your base exposes. And then, base will execute its constructor before you set your virtual member.

like image 41
T.S. Avatar answered Oct 16 '22 13:10

T.S.