25

I have a windows form, and I have a class that checks a text file to make sure it has certain aspect. Now I have methods in a constructor and it seems a little weird. Should I just leave the constructor blank, and implement a start() type method and call that? So far my code looks like this

public class Seating
{
    private int segments = 0;
    public Seating()
    {
        checkInvoice();
        getSegmentCount();          
    }
}
1
  • If you do initializations, it's common but seems to do validations either? Jan 5, 2011 at 14:16

7 Answers 7

35

I have methods in a constructor and it seems a little weird

Well, why else would a constructor exist? It’s entirely legitimate to have methods in a constructor, even methods that could fail (thus interrupting the object creation). It’s hard to say whether this is appropriate in your particular case.

In general it’s fine though.

14
  • okay perfect, I thought constructors were more for initializing variables, I suppose.
    – Spooks
    Jan 5, 2011 at 14:16
  • 5
    @Lucero: I disagree. The best practice is to keep the constructor cleaning (not clean) up after itself if it deals with disposable resources. Of course the uncreated object must not leak resources but it’s entirely possible to prevent this in the constructor itself (and should absolutely be done!). A constructor should do all the work it meaningfully has to do to get the object in an initialized state. If that entails dealing with code that can throw exceptions, so be it. If the constructor leaves the object half initialized (as suggested by you) then it is broken. Jan 5, 2011 at 14:21
  • 2
    @Lucero: but notice that Herb’s article in particular doesn’t say that the consequence is to cripple constructors. He just says to beware of incorrect handing. The second and third links do not forward any argument either way, they just list the options (although the third link explicitly warns of the danger of partly constructed objects that you are advocating). All three articles are interesting reads but none supports your argument of crippling constructors (what you called “clean and lean”). Jan 5, 2011 at 15:00
  • 1
    @Konrad, I called you arrogant for putting words in my mouth, not for standing your point. But back on topic: if I understand you correctly, you also find WebRequest/WebResponse descendants, the complete XML DOM, network sockets and many more APIs are badly designed? All those do work with the create-setup-use workflow (vs. create-use only). I see benefits in both and for me there is no smell in either approach. A partly initialized object is uglier (readonly fields which may be null even though the constructor seems to always assign a value and such) than one which was not yet set up.
    – Lucero
    Jan 6, 2011 at 20:36
  • 2
    @Lucero: to answer very briefly: yes. They are all badly designed. Setting up an object should be reserved for a builder instance. The point is that not all of this was known when the program was initially designed. We learn from our (and other peoples’) mistakes. Jan 6, 2011 at 20:57
13

There's nothing wrong with calling non-virtual methods in the constructor. By the time your constructor fires, your parent object has been fully constructed. However, you do not want to call any virtual methods, as they could be overridden by subclasses, which would execute code within them when they are not fully constructed.

1
  • 3
    There's no problem with calling virtual members in the constructor if they do not use any instance members that have to be initialized. Jan 26, 2014 at 22:42
10

The general rule is have methods that are very trivial and unlikely to throw exceptions in constructor. If the methods might fail for any reason (file access or missing, DB issue, null reference ...), then it should not be in the constructor, as people should expect constructors not to fail (especially parameterless constructors, although the general guidance doesn't specify). People should not expect var seating = new Seating(); to be source of errors.

You can add Start() / Initialize() kind of methods as instance methods as you mentioned. You can also add a new static method that returns a new instance of the Class after calling the two methods you need on it and make the constructor private. You can go further and have this method in a new Factory class (making the constructor internal). You can think of other ways to do it also.

One thing is you may see any value calculated or retrieved in these methods a constructor parameter (and have no parameterless constructor), then the constructor will only assign those parameters to respective fields/properties. A factory method or service method in the same assembly or other "Services" dedicated assembly can be responsible for calling the methods, getting the parameters, passing them to the constructor, and returning a new instance of the class. This is my personal favorite.

Generally speaking this kinds of problems is a sign that the class is doing too much and you might want to split the functionality into other classes (existing or new). That's why the last solution is suggested, so that the two methods themselves might be in the other "Services" assembly, but as mentioned above, there are many other ways to do it if you want.

Update:

Here is Microsoft Guidelines for Constructors:
Constructor Usage Guidelines

Quoting from the page:

Minimize the amount of work done in the constructor. Constructors should not do more than capture the constructor parameter or parameters. This delays the cost of performing further operations until the user uses a specific feature of the instance.

Update 2

The above page moved to a living document (which means it's updatable) under the name Constructor Design.

4
  • 3
    This is wrong. It might help if you gave reasons why you think that “People should not expect var seating = new Seating(); to be source of errors.” – in general, there is no reason why they should not. Introducing a Start method doesn’t make this any better (worse, in fact). Having an object in an improperly initialized state is an anti-pattern and although many older APIs do that, you should not. Jan 7, 2011 at 13:57
  • 1
    Why? Because it seems like just creating instance of class. Nothing special. No constructor parameter or anything that feels like it does something heavy. I did not recommend the Start method, I mentioned my suggestion is the last option. A constructor that takes parameters and a service method which calls the two methods, provides parameters to the constructor, and returns the new instance. Read it again.
    – Meligy
    Jan 8, 2011 at 4:27
  • See the "Update" part of the answer, about Microsoft's own guidelines for constructors.
    – Meligy
    Jan 8, 2011 at 4:30
  • 2
    The constructor usage guidelines you cite were written for .NET 1.1 and do not really say anything about exceptions. On the other hand, these constructor design guidelines for .NET 4.5 mention "DO throw exceptions from instance constructors, if appropriate." Jan 26, 2014 at 22:38
8

Generally speaking, your constructor should only be setting up the state of the object. It is possible that could mean calling methods. I would say that running a very long methods is not a good idea. The user of your class wouldn't expect the constructor to be an expensive method.

I don't you didn't mention virtual methods, but you should NEVER call these in a constructor.

Here is an example of how it can get back quickly:

public class MyBase
{
        protected MyBase()
        {
                this.VirtualMethod();
        }

        protected virtual void VirtualMethod()
        {
                Console.WriteLine("VirtualMethod in MyBase");
        }
}

public class MyDerived : MyBase
{
        private readonly string message = "Set by initializer";

        public MyDerived(string message)
        {
                this.message = message;
        }

        protected override void VirtualMethod()
        {
                Console.WriteLine(this.message);
        }
}

Now, lets say you have this code elsewhere:

MyDerived d = new MyDerived("Called from constructor");

What do you think will be shown on the console? If you said "Set by initializer", then you are correct.

This is why:

  • All of the field initializers are executed before the code in the constructor.
  • The C# compiler adds a call to the base constructor before anything that is user defined. In this case, it calls up to
    MyBase which calls the VirtualMethod(). Since the runtime type of d is MyDerived, the override of VirtualMethod() in MyDerived is
    executed. And now since the body of MyDerived's constructor hasn't
    executed yet, this.message has the value it was given in the
    initalizer.
  • Now the body of MyDerived's constructor is executed.
  • Later calls to VirtualMethod() on this instance will now print out
    "Called from constructor".
5
  • There's no problem with calling virtual members in the constructor if they do not use any instance members that have to be initialized. Jan 26, 2014 at 22:43
  • Disagree. This pattern is just asking for trouble. You never know who will inherit from your class, override that virtual method and do bad stuff.
    – Bryan
    Jan 28, 2014 at 19:12
  • That's what the API docs are there for. I am all for installing compile-time safeguards, but as I pointed out in my comment to another answer here, initializing readonly fields in an overrideable way via constructor arguments instead of virtual method calls just opens up other "design vulnerabilities". While inheritance in general provides elegant solutions, it always opens up certain ways for malicious inheritors to misbehave - think of overridden methods returning null where an instance is direly needed, or of overridden methods invoking Dispose on some other member. Jan 28, 2014 at 19:36
  • I added an example of something very bad.
    – Bryan
    Feb 21, 2014 at 22:19
  • Thanks for the example. As I said, "That's what the API docs are there for." You conveniently left those out, especially the one for MyBase.VirtualMethod, which would realistically point out that overridden versions of that method must not access any instance members. With that in mind, it becomes obvious that MyDerived.VirtualMethod violates constraints from the API specification and therefore cannot be expected to behave as intended (assuming the intention was to output "Called from constructor"). Feb 22, 2014 at 10:10
5

Virtual method calls in the constructor are no-go (with the tiny exception of sealed classes, which makes the method effectively non-virtual).

Non-virtual methods can be okay, but you should make sure that all members have been initialized before, and the non-virtual methods may not call any virtual member as well.

3
  • I wouldn't quite say that virtual method calls in the constructor are a no-go (actually, there doesn't seem to be any other way to initialize readonly fields in an overridable way); those virtual methods just may have to check whether certain fields that are otherwise "always" initialized are indeed initialized. Virtual methods that do not use any instance members are even entirely fine in that respect. Jan 26, 2014 at 22:34
  • The problem is that the virtual method call breaks common expectations when called in the constructor, and any change in the call order of the base constructor may change what fields are (or are not) initialized when the virtual method is being called. Since making methods virtual is exposing a member for extension in deriving classes (maybe by users not aware of the special conditions), this is not a good practice. An viable option to initialize readonly fields in an "overridable" way is often to provide a protected constructor receiving the values for these fields.
    – Lucero
    Jan 27, 2014 at 9:44
  • Users can simply be made aware of the special conditions by the documentation of the respective method. As for the protected constructors, it seems to me that on the one hand, parametrized virtual methods are problematic in that respect, while return values parameterless methods could indeed be passed as constructor parameters, though one would lose a strong design hint that the value is supposed to differ by class, not by instance ("malicious" inheritors could expose the protected constructor's default value parameter in a public constructor). Jan 27, 2014 at 9:54
2

There are some useful design guidelines for constructor usage here:

Constructor Design (MSDN)

The text is lifted from:

Framework Design Guidelines: Conventions, Idioms, and Patterns for Reusable .NET Libraries

Which is a worthy purchase.

The one I got bitten by years ago was: "Do not call virtual members on an object inside its constructors."

1
  • Calling virtual members in a constructor can be done in a safe way, and there certainly are scenarios when that is appropriate (such as initializing readonly fields in an overrideable way). Interestingly, in the .NET 4.5 version of the guidelines you cite, that DO NOT was changed into an AVOID, i.e. "Try not to, but do if you really need to." Jan 26, 2014 at 22:42
0

You’re free to put as much code in there as you like, but by convention, developers usually expect the constructor to do very little — its main job is to ensure that the object is in a valid initial state. That might involve checking the arguments and throwing an exception if there’s a problem, but not much else. You are likely to surprise developers who use your class if you write a constructor that does something nontrivial, such as adding data to a database or sending a message over the network

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.