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();          
    }
}
Spooks
  • 6,937
  • 11
  • 49
  • 66

7 Answers7

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.

Konrad Rudolph
  • 530,221
  • 131
  • 937
  • 1,214
  • okay perfect, I thought constructors were more for initializing variables, I suppose. – Spooks Jan 05 '11 at 14:16
  • Handling failures in constructors is a difficult topic. Since the reference is not retunrd to the code which initiated the object creation, it cannot do a cleanup (e.g. call the `Dispose()` method). And even if it could (or if it is finalized), the cleanup code must be aware that the object may not be fully intialized yet. The best practice is to keep the construtor clean and lean because this avoids many difficulties. – Lucero Jan 05 '11 at 14:18
  • 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. – Konrad Rudolph Jan 05 '11 at 14:21
  • @Konrad, I wrote that is is difficult to get right. Blog posts like those illustrate what I mean: http://herbsutter.com/2008/07/25/constructor-exceptions-in-c-c-and-java/ or http://tekpool.wordpress.com/2006/12/08/constructors-error-exception-handling-in-constructors/ and many more... (I think cbrumme wrote about it some time ago, but I didn't find this one right away) – Lucero Jan 05 '11 at 14:28
  • ...and this one by Joe Duffy is really worth a read: http://www.bluebytesoftware.com/blog/2010/06/27/OnPartiallyconstructedObjects.aspx – Lucero Jan 05 '11 at 14:38
  • 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”). – Konrad Rudolph Jan 05 '11 at 15:00
  • @Konrad, "crippling constructors" is your interpreation. I'm trying to point out that correct handling of these cases is not really trivial, and therefore in the case of doubt I'd apply the KISS principle. This is what I meand by clean and lean. If someone is unsure whether using methods in constructors is OK, chances are good that the person is not aware of the implications with partially constructed objects and proper error handling... – Lucero Jan 05 '11 at 17:08
  • @Lucero: according to your third link, chances are the person can safely ignore those potential problems. You are not making the code any simpler, and your method doesn’t make the problem go away. Instead, you leave the user with a half initialized object that cannot yet be used, creating more administrative overhead (with the potential of making mistakes) for the user. You’re not mitigating the problem, just pushing it off to the client of your API. In brief, you’re robbing the constructor of its sole purpose: providing the client with a usable object. – Konrad Rudolph Jan 05 '11 at 20:13
  • @Konrad, what are you talking about? My method? Not making the code any simpler? Leave the user with half-initialized objects? Robbing the constructur of its sole purpose? I really don't know where you're taking this from. In fact, my intention was and is exactly to make the op aware of these problems related to calls on half-initialized objects, be it through virtual member calls, finalizers, unhandled exceptions etc. in the constructor. Since I don't like having people pretend things I never even thought of as being coming from me I'm not going to continue this discussion with you. – Lucero Jan 05 '11 at 20:38
  • @Lucero: your method = not initializing the object fully in the constructor by not calling methods that could throw exceptions. After a constructor has run, the object has to be completely initialized. Period. – Konrad Rudolph Jan 05 '11 at 20:55
  • @Konrad, I never wrote that it should *not be fully initialized*! Full initialization does not imply allocating all resources. It's a design decision, just like a `SqlConnection` which needs to be opened after it has been created. Would you say that the connection object is not fully initialized after creation? If you want to quote something that I wrote that's fine with me. But stop pretending that I have specific "methods" etc. if that is not the case; it does disqualify you because that's just your personal arrogant interpreation of what you think I meant. – Lucero Jan 06 '11 at 13:42
  • @Lucero: careful with interpretations indeed: Yes, `SqlConnection` *isn’t* fully initialized after creation and it’s a badly designed API. This *is* what I mean and it’s code smell; it makes no sense to have a connection that isn’t, well, *connected*, does it? As an example of a class that does it better, look at `FileStream` (and other IO classes) … here the constructor opens the file. Call me arrogant all you like but I stand by my point. – Konrad Rudolph Jan 06 '11 at 14:59
  • 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 06 '11 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. – Konrad Rudolph Jan 06 '11 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.

Adam Robinson
  • 182,639
  • 35
  • 285
  • 343
  • 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. – O. R. Mapper Jan 26 '14 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.

Meligy
  • 35,654
  • 11
  • 85
  • 109
  • 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. – Konrad Rudolph Jan 07 '11 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 08 '11 at 04:27
  • See the "Update" part of the answer, about Microsoft's own guidelines for constructors. – Meligy Jan 08 '11 at 04: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](http://msdn.microsoft.com/en-us/library/ms229060%28v=vs.110%29.aspx) for .NET 4.5 mention "**DO** throw exceptions from instance constructors, if appropriate." – O. R. Mapper Jan 26 '14 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".
Bryan
  • 2,775
  • 3
  • 28
  • 40
  • There's no problem with calling virtual members in the constructor if they do not use any instance members that have to be initialized. – O. R. Mapper Jan 26 '14 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 '14 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. – O. R. Mapper Jan 28 '14 at 19:36
  • I added an example of something very bad. – Bryan Feb 21 '14 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"`). – O. R. Mapper Feb 22 '14 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.

Lucero
  • 59,176
  • 9
  • 122
  • 152
  • 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. – O. R. Mapper Jan 26 '14 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 '14 at 09: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). – O. R. Mapper Jan 27 '14 at 09: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."

Kev
  • 118,037
  • 53
  • 300
  • 385
  • 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." – O. R. Mapper Jan 26 '14 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

SGh
  • 137
  • 1
  • 2
  • 9