22

I am having a discussion with my co-worker on how much work a constructor can do. I have a class, B that internally requires another object A. Object A is one of a few members that class B needs to do its job. All of its public methods depends on the internal object A. Information about object A is stored in the DB so I try to validate and get it by looking it up on the DB in the constructor. My co-worker pointed out that the constructor should not do much work other than capture the constructor parameters. Since all of public methods would fail anyway if object A is not found using the inputs to the constructor, I argued that instead of allowing an instance to be created and later fail, it is actually better to throw early in constructor. Not all classes do this but i found that StreamWriter constructor also throws if it has problem opening a file and doesn't delay the validation until the first call to Write.

What others think? I am using C# if that makes any difference.

Reading Is there ever a reason to do all an object's work in a constructor? i wonder fetching object A by going to DB is part of "any other initialization necessary to make the object ready to use" because if user passed in wrong values to the constructor, i wouldn't be able to use any of its public methods.

Constructors should instantiate the fields of an object and do any other initialization necessary to make the object ready to use. This is generally means constructors are small, but there are scenarios where this would be a substantial amount of work.

Community
  • 1
Taein Kim
  • 331
  • 1
  • 2
  • 5
  • 8
    I disagree. Have a look at dependency inversion principle, it would be better if object A is handed to object B in a valid state in it's constructor. – Jimmy Hoffa Jan 13 '14 at 18:50
  • 5
    Are you asking how much **can** be done? How much **should** be done? Or how much **of your specific situation** should be done? That's three very different questions. – Bobson Jan 13 '14 at 18:52
  • 1
    I agree with Jimmy Hoffa's suggestion to look at dependency injection (or "dependency inversion"). That was my first thought on reading the description, because it sounds like you're already half way there; you already have functionality separated out into class A, which class B is using. I can't speak to your case, but I generally find that refactoring code to use the dependency injection pattern makes classes simpler / less intertwined. – Dr. Wily's Apprentice Jan 13 '14 at 19:43
  • 1
    Bobson, i changed the title to 'should'. I am asking for best practice here. – Taein Kim Jan 13 '14 at 20:50
  • I figured that might be the case, but it wasn't clear. Thanks for clarifying. – Bobson Jan 13 '14 at 20:52
  • JimmyHoffa is right. You should inject A into B. How will you test B if you need a database just to create one? – kevin cline Jan 13 '14 at 21:53
  • 1
    @JimmyHoffa: maybe you should expand your comment into an answer. – kevin cline Jan 13 '14 at 21:53
  • See also: [Object Initializer in C# problem with readability](http://programmers.stackexchange.com/q/201701/6605) – Arseni Mourzenko Jan 13 '14 at 21:58
  • [Microsoft guidelines](https://msdn.microsoft.com/en-us/library/vstudio/ms229060(v=vs.100).aspx) state "Do minimal work in the constructor" – BornToCode May 05 '15 at 10:30

3 Answers3

24

Your question is composed from two completely separate parts:

Should I throw exception from constructor, or should I have method fail?

This is clearly application of Fail-fast principle. Making constructor fail is much easier to debug compared to having to find why the method is failing. For example, you might get the instance already created from some other part of code and you get errors when calling methods. Is it obvious the object is created wrong? No.

As for the "wrap the call in try/catch" problem. Exceptions are meant to be exceptional. If you know some code will throw an exception, you do not wrap the code in try/catch, but you validate parameters of that code before you execute the code that can throw an exception. Exceptions are only meant as way to ensure the system doesn't get into invalid state. If you know some input parameters can lead to invalid state, you make sure those parameters never happen. This way, you only have to do try/catch in places, where you can logically handle the exception, which is usually on system's boundary.

Can I access "other parts of the system, like DB" from constructor.

I think this goes against principle of least astonishment. Not many people would expect constructor to access a DB. So no, you should not do that.

Euphoric
  • 36,735
  • 6
  • 78
  • 110
  • 2
    The more appropriate solution to this is usually to add an "open" type method where construction is free, but no use can be done before you "open"/"connect"/etc which is where all the actual initialization occurs. Constructors failing can cause all sorts of issues when you in the future want to do partial-construction of objects which is a useful ability. – Jimmy Hoffa Jan 13 '14 at 18:53
  • @JimmyHoffa I see no difference between constructor method and specific method for construction. – Euphoric Jan 13 '14 at 18:57
  • 5
    You may not, but many do. Think about when you create a connection object, does the constructor connect it? Is the object usable if it's not connected? In both questions the answer is no, because it's helpful to have connection objects partially-constructed so you can tweak settings and things before you open the connection. This is a common approach to this scenario. I still think constructor injection is the right approach for scenarios where object A has a resource dependency but an open method is still better than having the constructor do the dangerous work. – Jimmy Hoffa Jan 13 '14 at 18:59
  • @JimmyHoffa But that is specific requirement of connection class, not general rule of OOP design. I would actually call it an exceptional case, than a rule. You are basically talking about builder pattern. – Euphoric Jan 13 '14 at 19:11
  • 2
    It's not a requirement, it was a design decision. They could have made the constructor take a connection string and connect immediately when constructed. They decided not too because it's helpful to be able to create the object, then tweak figure out the connection string or other bits and bobbles and connect it *later*, – Jimmy Hoffa Jan 13 '14 at 19:14
  • I disagree with your exception handling philosophy. "You make sure those parameters never happen" In theory, you're right, but those params tend to find a way to happen anyway. There's either a case you didn't expect or, more likely, maintenance happens that now allows invalid values. E.G. Say a DB column your param originated from changes to nullable. The person who changed that may not know your constructor of this other class assumes it will never be null.Now your whole application may crash. Almost always better to handle exceptions that may be thrown. "Failing to plan is planning to fail" – xr280xr Mar 09 '17 at 19:24
  • @xr280xr If you change field into nullable, you have to change type, where every invalid usage will be caught by compiler. – Euphoric Mar 09 '17 at 20:13
  • @Euphoric - Not if it's already a nullable type with only the DB preventing nulls, such as a string. A trivial point because there are a million other ways an unrelated change could unwittingly change the assumptions your lack of exception handling is dependent on. I've seen code that was perfectly safe at the time it was written now blowing up time after time. There are simple cases where the risk of not handling exceptions is acceptable, but that must be weighed carefully and is less commonly the best option. – xr280xr Mar 09 '17 at 20:31
  • @xr280xr if you get an exception that you didn't expect, chances are that even if you catch it you do not know how to resume operation (because you don't know what caused the failure in the first place) and you can only bail out and raise the error to the user, or crash the program (or both) – pqnet Mar 07 '18 at 23:49
  • @pqnet , I find that in a business application, it's almost always better to raise an error to the user, even if you don't know what happened (and log what you do know) than to just crash the application. It has been an extremely rare case, in my experience, that I couldn't jump back out to a level of the application where it is still safe to continue execution. That's not to say it doesn't happen or is even always rare, just that it can be very common to be able to continue as well despite your remark being a very common stance on this matter. – xr280xr Mar 08 '18 at 00:01
  • **"Exceptions are meant to be exceptional. If you know some code will throw an exception, you do not wrap the code in try/catch, but you validate parameters of that code before you execute the code that can throw an exception."** Good to know, I'll make sure parameters `X`, `Y` and `Z` are valid every time I want to construct `Foo` with them. Of course this means I need to be aware of all the explicit and implicit requirements for them, and spread the check code all across the code base and pray the requirements for them never change - I'm sure the code will indeed be **exceptional** – CharonX Jan 27 '20 at 16:23
20

Ugh.

A constructor should do as little as possible - the issue being that exception behavior in constructors is awkward. Very few programmers know what the proper behavior is (including inheritence scenarios), and forcing your users to try/catch every single instantiation is... painful at best.

There's two parts to this, in the constructor and using the constructor. It's awkward in the constructor because you can't do much there when an exception occurs. You can't return a different type. You can't return null. You basically can rethrow the exception, return a broken object (bad constuctor), or (best case) replace some internal part with a sane default. Using the constructor is awkward because then your instantiations (and the instantiations of all derived types!) can throw. Then you can't use them in member initializers. You end up using exceptions for logic to see "did my creation succeed?", beyond the readability (and fragility) caused by try/catch everywhere.

If your constructor is doing non-trivial things, or things that can reasonably be expected to fail, consider a static Create method (with a non-public constructor) instead. It's much more usable, easier to debug, and still gets you a fully constructed instance when successful.

Telastyn
  • 108,850
  • 29
  • 239
  • 365
  • 2
    Complex construction scenarios are exactly why creational patterns like you mention here exist. This is a good approach to these problematic constructions. I'm given to think about how in .NET a `Connection` object can `CreateCommand` for you so that you know the command is appropriately connected. – Jimmy Hoffa Jan 13 '14 at 18:57
  • 2
    To this I would add that a Database is a heavy external dependance, suppose you want to switch databases at some point in the future (or mock an object for testing), moving this kind of code to a Factory class (or something like an IService provider) seems like a cleaner approach – Jason Sperske Jan 13 '14 at 18:58
  • Why would something force programmer to catch exceptions where it cannot be handled? Also, those "complex constructions" are usually reserved for need of data from other parts of the system. If you can have all data passed into constructor, there is no need to create specific constructor class. – Euphoric Jan 13 '14 at 18:59
  • 4
    can you elaborate on "exception behavior in constructors is awkward"? If you were to use Create, wouldn't you need to wrap it in try catch just like you would wrap call to constructor? I feel like Create is just different name for constructor. Maybe i am not getting your answer right? – Taein Kim Jan 13 '14 at 19:01
  • @user21479 - There's two parts to this, in the constructor and using the constructor. It's awkward _in_ the constructor because you can't do much there. You can't return a different object. You can't return null. You basically can rethrow the exception, return a broken object (bad constuctor), or (best case) replace some internal part with a sane default. _Using_ the constructor is awkward because then your instantiations (and the instantiations of all derived types!) can throw. Then you can't use them in member initializers. You end up using exceptions for logic, beyond try/catch everywhere. – Telastyn Jan 13 '14 at 19:18
  • So the major difference between ctor and Create is that ctor returns an instance of the class when it is successful whereas Create can return anything it wants to. With introduction of exception in C++, returning null due to an error is a bad practice. I also don't understand when you would want ClassB.Create to return something other than ClassB unless it is a factory class, which it isn't in this case. Also, I don't think there is any difference on 'using' side because you still have one try/catch whether you are using ctor or Create. Example would be useful. thanks! – Taein Kim Jan 13 '14 at 19:56
  • I agree that DB call probably is too heavy for ctor. I just want to make sure i understand your points correctly :) – Taein Kim Jan 13 '14 at 19:57
  • 1
    Having had to try catching exceptions bubbling up from constructors, +1 to the arguments against this. Having worked with folks that say "No work in the constructor" though, this can also create some weird patterns. I have seen code where every object has not only a constructor, but also some form of an Initialize() where, guess what? The object isn't really valid until that's called either. And then you have two things to call: the ctor and Initialize(). The problem of doing work to set up an object doesn't disappear - C# may yield different solutions than say C++. Factories are good! – J Trana Jan 14 '14 at 05:51
  • 1
    @jtrana - yeah, initialize is no good. The constructor initializes to some sane default or a factory or create method builds it and returns a usable instance. – Telastyn Jan 14 '14 at 12:24
0

Constructor - method complexity balance is indeed a matter of discussion about good stile. Quite often empty constructor Class() {} is used, with a method Init(..) {..} for more complicated things. Among arguments to be taken into account:

  • Class serialization possibility
  • With the separation Init method from constructor, you often need to repeat same sequence Class x = new Class(); x.Init(..); many times, partially in Unit Tests. Not so good, however, crystal clear for reading. Sometimes, I just put them in one line of code.
  • When Unit Test aim is just Class initialization test, better to have own Init, to do more complicated actions fulfilled one-by-one, with the intermediate Asserts.
Pavel Khrapkin
  • 9
  • 3
  • the advantage, in my opinion, of the `init` method is that handling failure is a lot easier. In particular, the return value of the `init` method can indicate whether initialization is successful, and the method can ensure the object is always in a good state. – pqnet Mar 12 '18 at 11:27
  • @pqnet With `init()` methods I see the issue that you end up with temporal coupling. You can do `X` with this class, but you have to `init()` it first. So now you need to add checks to every function call if the class is really initialized and handle the possible failure events... – CharonX Jan 28 '20 at 15:28
  • @pqnet: if init() fails, what then? Throw an exception? What's the advantage exactly? You just lost the stack trace along with everything else, and why couldn't that have been handled just as well in the constructor to begin with? Useless wrapper methods are a pain – Ed S. Oct 11 '22 at 00:15
  • @pqnet: "and the method can ensure the object is always in a good state" - like a constructor is meant to do? – Ed S. Oct 11 '22 at 01:13