c# call virtual/abstruct function from the constructor of the baseclass

[puzzlecracker]

Hello,

In general, it's a bad practice to call members in a derived class,
if the class hasn't been initialized. As a catchphrase goes, bad
things may happen. However, I think, my situation warrants to break
this rule, as I don't see anything going wrong. Plus, it really
alleviates my design constrains. Let me know what you think about
it.

Here is the example:

public interface IDisplay
{
public void Display();

}

public class XDisplay: IDisplay
{
public void Display() { /* display X widget */}

}

public class YDisplay: IDisplay
{
public void Display() { /* display Y widget */}
}

public abstract class ADisplayerPool
{
private IDisplay m_display=null;

protected abstract IDisplay CreateDisplay();

public AClass()
{
m_display =CreateDisplay();
}
}

public class DisplayPoolX : ADisplayerPool
{
protected IDisplay CreateDisplay() {return new XDisplay()}

}

public class DisplayPoolY : ADisplayerPool
{
protected IDisplay CreateDisplay() {return new YDisplay()}

}


Let me know if it's sound design or/and how you would restructure
it.

 

[puzzlecracker]

In practice, if you're very careful, you can get away with it.  But IMHO  
it's better to, in these situations, provide a post-constructor  
initialization method that does the polymorphic work.

Pete

Thanks Pete, I don't want to overburden users by calling a post-
initialization method, especially for things that don't directly
affect them. Initially, I was considering putting the call of this
method from other methods that user calls, and have initialization
flag set. However, I decided against it, as it will become a
maintenance hell down the road -- new methods will need to be updated
to call that polymorphic method, and uninformed programmer will not
know or forget.

 

[Peter Morris]

I would do

public abstract class ADisplayerPool
{
protected abstract IDisplay CreateDisplay();

private IDisplay display;
public IDisplay Display
{
get
{
if (display == null)
display = CreateDisplay();
return display;
}
}
}

 

[puzzlecracker]

public abstract class ADisplayerPool

{
    protected abstract IDisplay CreateDisplay();

    private IDisplay display;
    public IDisplay Display
    {
        get
        {
            if (display == null)
                display = CreateDisplay();
            return display;
        }
    }

}

Nice!!!! Why didn't I think of that. That's a very good solution.

 

[puzzlecracker]

Except that you are then overburdening inheriting classes by creating a  
trap for the unwary inheritor.

Elegant and easy is well and good, but bug-free is better.


I agree that's not an improvement.  It simply trades one maintenance  
problem for another.

If you are concerned so much about convenience, you can make your class  
constructors hidden, and use a factory method to create instances.  Then  
the factory method can perform both the construction and initialization.  
Clients will still only see a single statement to create the object, but  
you'll be able to ensure that the polymorphic stuff happens only after  
construction.

Pete

I wish I could change the client's interface, but I can't, as we
already have the interface in the production for many years. I usually
prefer using factories when coding client's api since it provides
lot's benefits among those you've mentioned.

 

[Jeff Louie]

Peter... Is this thread safe?

Regards,
Jeff

public IDisplay Display
{
get
{
if (display == null)
display = CreateDisplay();
return display;
}
}
}

 

[Arne Vajhøj]

Is this thread safe?

public IDisplay Display
{
get
{
if (display == null)
display = CreateDisplay();
return display;
}
}
}

If you google for "C# singleton thread safe", then you
will learn a lot about this particular problem.

Arne

 

[Arne Vajhøj]

At the very least, the "display" variable needs to be volatile for that
code to be thread-safe. And even then, you risk initializing "display"
more than once (in some situations, multiple initializations are
harmless, even if less efficient). If you want (or need) to prevent
that, you need to synchronize the code. For example:

private object _objDisplayLock = new object();

public IDisplay Display
{
get
{
lock (_objDisplayLock)
{
if (display == null)
{
display = CreateDisplay();
}
}

return display;
}
}

Note that if you synchronize access with "lock", you don't need to make
the variable "volatile".

And even though some people like to write weird code to avoid the
lock in their lazy loaded singleton, then the code above is very
readable/maintainable and will most likely perform good enough.

Arne

 

[Jeff Louie]

Hi Arne... I was actually hoping that I could gently prompt the author
of the
code into realizing the potential problem

Regards,
Jeffwill learn a lot about this particular problem.<<

 

[Peter Morris]

Peter... Is this thread safe?

If he had specified that it needed to be thread safe I would have written it
differently.

 

[Arne Vajhøj]

Most code is not thread-safe. Lack of thread-safety is not a problem,
unless there is a requirement to be thread-safe.

Code fragments are rarely thread safe in all contexts. Whether
something is thread safe or not depends on the context. Which
is why I would express the substance of what you write as that
most code is thread safe in their context without doing anything.
And even though it may appear absurd to consider all code thread
safe in a single threaded context, then I think it reflects reality.

Arne