Good design or bad design of abstract class?

I came across this statement in the Framework Design Guidelines, page 84:

DO NOT define public or protected-internal constructors in abstract types.”

Then it reads:

DO define a protected or an internal constructor on abstract types.”

Sure. Makes perfect sense.  Then come the examples.  It says the following code is good design:

Good design

            public abstract class MySecondAbstract

            {

                        protected MySecondAbstract()

                        {

                        }

            }

So far so good.  Everything I’ve shown and read so far is something I think we’ve all known and agreed with for many, many years.  Its basic abstract class design.

This next bit of code is labeled as good design/incorrect design:

Good design/incorrect design

            // good design

            public abstract class MyFirstAbstract

            {

                        protected int _userId;

 

                        // incorrect design

                        protected MyFirstAbstract(int userId) // <— this line is bad

                        {

                                    _userId = userId;

                        }

            }

Why would this be bad design, as the book states?  Simple constructors are fine.  Protected constructors for abstract classes are fine.  So whats the deal?

At first the thought was, maybe its creating a default public parameterless constructor that gets exposed.  But, the compiler is smarter than that and its not.  So far the only thing I can figure out is that design guidelines require that a parameterless, protected constuctor always exist for an abstract type.

What are your thoughts on this?  Why would the above abstract class MyFirstAbstract be bad design?

This entry was posted in .Net Development, Patterns and Practices. Bookmark the permalink. Follow any comments here with the RSS feed for this post.

16 Responses to Good design or bad design of abstract class?

  1. Ur@ says:

    And what to do when userId is readonly Field?
    public abstract class MyFirstAbstract
    {
    protected READONLY int _userId;
    // incorrect design??
    protected MyFirstAbstract(int userId) // <— this line is bad
    {
    _userId = userId;
    }
    }

  2. Xanaris,

    I actually use the other options you have described in other places because they are more appropriate for the tasks and design.

    “If the base and derived classes are under my control and I am not publishing an API, I would most certainly use the approach you describe. ”

    In my specific case for the example I stated where I’m checking the validity, this is indeed the case, and why I use the approach. I’m going to try to get Brad Abrams to comment on this directly.

  3. xanaris says:

    Raymond,
    I see a number of options.

    If the parameter is not needed at all by base class and we just want to enforce validation rules when it is set, we could do:
    public abstract MyFirstAbstract()
    {
    protected MyFirstAbstract()
    {
    }

    private _userId;
    protected int UserId
    {
    get { return _userId; }
    set
    {
    if (value < 0) throw new ArgumentOutOfRangeException(“userId”);
    _userId = value;
    }
    }
    }

    If the validation is complex (also the initialization will be complex and chatty) we can implement validation methods:

    public abstract MyFirstAbstract()
    {
    protected MyFirstAbstract()
    {
    }

    protected abstract in UserId { get; set; }

    protected void SelfValidate()
    {
    if (userId < 0) throw new MyCustomValidationException();

    DoMoreValidation();
    }

    protected virtual void DoMoreValidation()
    { // inheritors add validation logic
    }

    public void SomeMethodThatRequiresValidState()
    {
    Selfvalidate();
    ….
    }
    }

    Finally, I may choose to ignore this rule – depending on the situation – if it causes much more problems than it solves. If the base and derived classes are under my control and I am not publishing an API, I would most certainly use the approach you describe.

  4. Xanaris,

    But what If I wanted to do something like so:

    public abstract MyFirstAbstract()
    {
    protected MyFirstAbstract(int userId)
    {
    if (userId < 0)
    throw new ArguementOutOfRangeException(“userId”);
    }
    }

    Would you really transfer that to each derived class? You are correct that in its required by the derived classes and not the base class, but what would you do about verifying the initialization and checking of required parameters? Personally, this is what I use the parameterized constructor in abstract classes for in many cases.

  5. xanaris says:

    Raymond,
    If the domain requires that every initialization provides this parameter, it sounds like it is mainly needed by derived classes and the base class may or may not have significant use for it. So why not transferring the responsibility to derived classes but in a way that does not limit their options. Something like the following class enables inheritors to choose how to initialize this parameter (constructor supplied, AppConfig, IoC etc.).
    public abstract MyFirstAbstract()
    {
    protected MyFirstAbstract()
    {
    }

    protected abstract object MyRequiredParam { get; }
    }

  6. Xanaris:

    Very well put and I completely understand that you are creating a more tightly coupled scenario between base and derived classes in this case. However, what if in the domain language itself, this scenario is a requirement? What if there shouldn’t be any type of initialization without the existing parameter?

    Frans:
    Serialization is an excellent point for needing to add the parameterless constructor.

    “So if initializing the class is mandatory, I’m now not so sure anymore if the design is that bad, as how else are you going to enforce calls to the initialization code without a constructor like the ‘bad design’ example.”

    This is my thought exactly. The book is overal generic in nature, naturally, because its framework design. Noting that, in general, the parameterized constructor isn’t the best design because of the coupling you have created. I think it is a necessary design in certain circumstances though, and shouldn’t be labeled as bad design, but perhaps as not preferred design.

  7. Frans Bouma says:

    exactly, xanaris, very well worded :)

    I now wonder though, if it wouldn’t be better to replace the protected constructor with a method like
    protected void InitClass(int userId)
    {

    }

    Because due to the parameter less constructor, initializing the class isn’t mandatory anyway.

    So if initializing the class is mandatory, I’m now not so sure anymore if the design is that bad, as how else are you going to enforce calls to the initialization code without a constructor like the ‘bad design’ example.

  8. xanaris says:

    I agree with Frans, the problem is that there is no default constructor in the base class and we should add a protected one. I don’t think the main reason is that in some situation, such as serialization, you need a no-param constructor. The main reason is that this framewiork is publishing an abstract class that will be inherited by users of the framework. Having only a parametrized constructor, we force all inheritors to supply this parameter, thus the signature of their constructors should include the parameter required by base class. That way we couple inheritors with the implementation details of the base class and we create an inflexible situation.

  9. Frans Bouma says:

    to add to corneliu’s comments:
    With the ‘bad’ design, you can never add an empty constructor without cooking up a userid to pass to the base constructor. because adding
    public MyFirstAbstract()
    {
    }

    won’t compile. In some scenario’s you need an empty constructor (xmlserializer etc. ) and you don’t know that in the abstract class, so you better be prepared for these kind of situations when you write your abstract class, hence it’s better to add an empty constructor.

  10. Corn,

    The book doesn’t actually list a field at all and still says the design is incorrect. It explicity says that the parameterized constructor, regardless of what the parameter is used for, is incorrect design. I absolutely see what you are talking about though, and its an excellent point.

  11. Corneliu says:

    Second example is bad design because the userId is marked as protected.
    If it’s protected, it means it can be safely modified by the interitor, thus the abstract should not depend on it or on having a valid/correct value.
    By making the constructor require the userId you mandate the inheritor giving you a value and “you need and you depend on it”, while you actually don’t.
    Making the member private, private readonly or protected readonly would make your design correct implying that userId is a mandatory value required in order to make your abstract class behave correclty.

    I think the constructor with the least no of parameters should be the one that defines the minimum variables/values that are required in order to guarantee that your class works correctly.

  12. _old _habits _are _hard _to _break

  13. I’ve been thinking about it some more and I just think it has to be some oversight in editing…there’s maybe missing information in the book. Because the “bad design” is really not apparrent in that snippet. And if it is the default parameterless constructor thing then certainly that should have been made more clear…but like the above guy said…that’s hard-core nit-picky.

    Or it could be that only losers use underscore prefixes.

  14. Fregas says:

    I’m all into good OO design, but that sounds like some nit-picky shit.

  15. Eber,

    The exception in there was misleading. Its not the actual problem. The exception was actually allowable because of its purpose of checking the parameter. I removed the exception being thrown to not cause further confusion. The problem is the constructor signature itself, according to the guidelines, not what is going on inside of it.

  16. mmm… you should not throw exceptions in the contructors?

Leave a Reply