22

I am working on a design, but keep hitting a roadblock. I have a particular class (ModelDef) that is essentially the owner of a complex node tree built by parsing an XML schema (think DOM). I want to follow good design principles (SOLID), and ensure that the resulting system is easily testable. I have every intention of using DI to pass dependencies into the constructor of ModelDef (so that these can easily be swapped out, if need be, during testing).

What I'm struggling with, though, is the creation of the node tree. This tree is going to be made up entirely of simple "value" objects which will not need to be independently tested. (However, I may still pass an Abstract Factory into ModelDef to assist with the creation of these objects.)

But I keep reading that a constructor should not do any real work (e.g. Flaw: Constructor does Real Work). This makes perfect sense to me if "real work" means constructing heavy-weigh dependent objects that one might later want to stub out for testing. (Those should be passed in via DI.)

But what about light-weight value objects such as this node tree? The tree has to be created somewhere, right? Why not via the constructor of ModelDef (using, say, a buildNodeTree() method)?

I don't really want to create the node tree outside of ModelDef and then pass it in (via constructor DI), because creating the node tree by parsing the schema requires a significant amount of complex code -- code that needs to be thoroughly tested. I don't want to relegate it to "glue" code (which should be relatively trivial, and will likely not be directly tested).

I have thought of putting the code to create the node tree in a separate "builder" object, but hesitate to call it a "builder", because it doesn't really match the Builder Pattern (which seem to be more concerned with eliminating telescoping constructors). But even if I called it something different (e.g. NodeTreeConstructor), it still feels like a bit of a hack just to avoid having the ModelDef constructor build the node tree. It has to be built somewhere; why not in the object that's going to own it?

  • 7
    You should always be wary of blanket statements like that, though. The general rule of thumb is code in a manner that is clear, functional, easy to test, reuse, and maintain, whatever manner that may be, which varies depending on your situation. If you gain nothing but code complexity and confusion trying to follow a "rule" like that then it wasn't an appropriate rule for your situation. All of these "patterns" and language features are tools; use the best one for your specific job. – Jason C Dec 20 '15 at 14:01
  • possible duplicate of Is there ever a reason to do all an object's work in a constructor? – gnat Dec 21 '15 at 21:12
26

And, besides what Ross Patterson suggested, consider this position which is the exact opposite:

  1. Take maxims such as "Thou Shalt Not Do Any Real Work In Thy Constructors" with a grain of salt.

  2. A constructor is, really, nothing but a static method. So, structurally, there is really not much difference between:

    a) a simple constructor and a bunch of complex static factory methods, and

    b) a simple constructor and a bunch of more complex constructors.

A considerable part of the negative sentiment towards doing any real work in constructors comes from a certain period of the history of C++ when there was debate as to precisely what state the object will be left in if an exception is thrown within the constructor, and whether the destructor should be invoked in such an event. That part of the history of C++ is over, and the issue has been settled, while in languages like Java there never was any issue of this kind to begin with.

My opinion is that if you simply avoid using new in the constructor, (as your intention to employ Dependency Injection indicates,) you should be fine. I laugh at statements like "conditional or looping logic in a constructor is a warning sign of a flaw".

Besides all that, personally, I would take the XML parsing logic out of the constructor, not because it is evil to have complex logic in a constructor, but because it is good to follow the "separation of concerns" principle. So, I would move the XML parsing logic into some separate class altogether, not into some static methods that belong to your ModelDef class.

Amendment

I suppose that if you have a method outside of ModelDef which creates a ModelDef from XML, you will need to instantiate some dynamic temporary tree data structure, populate it by parsing your XML, and then create your new ModelDef passing that structure as a constructor parameter. So, that could perhaps be thought of as an application of the "Builder" pattern. There is a very close analogy between what you want to do and the String & StringBuilder pair. However, I have found this Q&A which seems to disagree, for reasons which are not clear to me: Stackoverflow - StringBuilder and Builder Pattern. So, to avoid a lengthy debate over here as to whether the StringBuilder does or does not implement the "builder" pattern, I would say feel free to be inspired by how StrungBuilder works in coming up with a solution that suits your needs, and postpone calling it an application of the "Builder" pattern until that little detail has been settled.

See this brand new question: Programmers SE: Is “StringBuilder” an application of the Builder Design Pattern?

  • 3
    @RichardLevasseur I just remember it as being a topic of concern and debate among C++ programmers in the early to mid nineties. If you look at this post: gotw.ca/gotw/066.htm you will see that it is fairly complicated, and fairly complicated stuff do tend to be controversial. I do not know for sure, but I think that in the early nineties part of that stuff had not even been standardized yet. But sorry, I cannot provide a good reference. – Mike Nakis Dec 20 '15 at 3:20
  • 1
    @Gurtz I would think of such a class as application-specific "xml utilities", since the format of the xml file (or the structure of the document) is probably tied to the particular application that you are developing, regardless of any possibilities to reuse your "ModelDef". – Mike Nakis Dec 20 '15 at 3:22
  • 1
    @Gurtz so, I would probably make them instance methods of the main "Application" class, or if that is too much hassle, then static methods of some helper class, in a way very similar to what Ross Patterson suggested. – Mike Nakis Dec 20 '15 at 3:36
  • 1
    @Gurtz apologies for not specifically addressing the "builder" approach earlier. I amended my answer. – Mike Nakis Dec 20 '15 at 15:26
  • 3
    @Gurtz It is possible but outside of academic curiosity, it doesn't matter. Don't get sucked in to the "pattern anti-pattern". Patterns are really just names to describe common / useful coding techniques to others conveniently. Do what you need to do, slap a label on it later if you need to describe it. It's perfectly OK to implement something that is "sort of like the builder pattern, in a way, maybe", as long as your code makes sense. It's reasonable to focus on patterns when learning new techniques, just don't fall into a trap of thinking that everything you do must be some named pattern. – Jason C Dec 21 '15 at 2:27
9

You already give the best reasons to not do this work in the ModelDef constructor:

  1. There's nothing "lightweight" about parsing an XML document into a node tree.
  2. There's nothing obvious about a ModelDef that says it can only be created from an XML document.

It sounds like your class should have a variety of static methods like ModelDef.FromXmlString(string xmlDocument), ModelDef.FromXmlDoc(XmlDoc parsedNodeTree), etc.

  • Thanks for the reply! Regarding the suggestion of static methods. Would these be static factories that create an instance of ModelDef (from the various xml sources)? Or would they be responsible for loading an already-created ModelDef object? If the latter, I'd be concerned about having the object only partially initialized (since a ModelDef needs a node tree to be fully initialized). Thoughts? – Gurtz Dec 20 '15 at 2:00
  • 3
    Excuse me for butting in, but yes, what Ross means is static factory methods that return fully constructed instances. The full prototype would be something like public static ModelDef createFromXmlString( string xmlDocument ). This is fairly common practice. Sometimes I do it too. My suggestion that you can also do just constructors is a standard type of response of mine in situations where I suspect that alternative approaches are dismissed as "not kosher" without good reason. – Mike Nakis Dec 20 '15 at 3:30
  • 1
    @Mike-Nakis, thanks for clarifying. So in this case the static factory method would build the node tree, and then pass that into the constructor (possibly private) of ModelDef. Make sense. Thanks. – Gurtz Dec 20 '15 at 12:40
  • @Gurtz Exactly. – Ross Patterson Dec 20 '15 at 22:07
5

I've heard that "rule" before. In my experience it is both true and false.

In more "classic" object orientation we talk about objects encapsulating state and behaviour. Thus an objects constructor should ensure that the object is initialized to a valid state (and signal a fault if the provided arguments does not make the object valid). Ensuring that an object is initialized to a valid state sure sounds like real work to me. And this idea has merits, if you have an object that only allows initialization to a valid state via the constructor and the object properly encapsulates it state so that each method that changes the state also checks that it doesn't change the state to something bad...then that object in essence guarantees that it is "always valid". That is a really nice property!

So the problem generally arrives when we try to break everything apart into small pieces to test and mock stuff. Because if an object is really properly encapsulated then you can't really get in there and replace the FooBarService with your mocked FooBarService and you (probably) can't just change values willy-nilly to suit your tests (or, it may take a lot more code than a simple assignment).

Thus we get the other "school of thought", which is SOLID. And in this school of thought it is most likely a lot more true that we shouldn't do real work in the constructor. SOLID code is often (but not always) more easy to test. But can also be harder to reason about. We break apart our code into small objects with a single responsibility, and thus most of our objects no longer encapsulate their state (and contain generally either state or behaviour). Validation code is generally extracted into a validator class and kept separate from the state. But now we've lost cohesion, we can no longer be sure that our objects are valid when we get them and to be completely sure we must always validate that the preconditions that we think that we have about the object is true before we attempt to do something with the object. (Of course, in general you do validation in one layer and then assume that the object is valid at lower layers.) But it's easier to test!

So who is right?

No one really. Both schools of thought have their merits. Currently SOLID is all the rage and everyone is talking about SRP and Open/Closed and all of that jazz. But just because something is popular doesn't mean that it is the correct design choice for every single application. So it depends. If you're working in a code base that is heavily following the SOLID principles then yes, real work in the constructor is probably a bad idea. But otherwise, look at the situation and try to use your judgement. What properties does your object gain from doing work in the constructor, what properties does it lose? How well does it fit with the overall architecture of your application?

Real work in the constructor isn't an antipattern, it can be quite the opposite when used in the correct places. But it should be documented clearly (along with which exceptions can be thrown, if any) and as with any design decision - it should fit within the general style used in the current code base.

  • This is a fantastic answer. – jrahhali Mar 15 '18 at 1:39
0

There is a fundamental problem with this rule and it's this, what constitutes "real work"?

You can see from the original article posted in the question that the author attempts to define what "real work" is, but it's severely flawed. For a practice to be good it needs to be a well defined principle. By that I mean that as respects to software engineering the idea should be portable (agnostic to any language), tested, and proven. Most of what is argued in that article doesn't fit into that first criteria. Here are some indicators the author mentions in that article of what constitutes "real work" and why they are not bad definitions.

Use of the new keyword. That definition is fundamentally flawed because it's domain specific. Some languages don't use the new keyword. Ultimately what he's hinting at is that it should not be constructing other objects. However in many languages even the most basic values are themselves objects. So any value assigned in the constructor is also constructing a new object. That makes this idea limited to certain languages, and a bad indicator as to what constitutes "real work".

Object not fully initialized after the constructor finishes. This is a good rule, but it also contradicts several of the other rules mentioned in that article. A good example of how that could contradict the others is mentioned the question that brought me here. In that question someone is concerned about using the sort method in a constructor in what appears to be JavaScript because of this principle. In this example the person was creating an object that represented a sorted list of other objects. For purpose of discussion imagine that we had a unsorted list of objects and we needed a new object to represent a sorted list. We need this new object because some part of our software is expecting a sorted a list, and lets call this object SortedList. This new object accepts an unsorted list and the resulting object should represent a now sorted list of objects. If we were to follow the other rules mentioned in that doc, namely no static method calls, no control flow structures, nothing more than assignment, then the resulting object would not be constructed in a valid state breaking the other rule of it being fully initialized after the constructor finishes. To fix this we would need to do some basic work to make the unsorted list sorted in the constructor. Doing this would break the 3 other rules, making the other rules irrelevant.

Ultimately this rule of not doing "real work" in a constructor is ill defined and flawed. Trying to define what "real work" is an exercise in futility. The best rule in that article is that when a constructor finishes it should be fully initialized. There are a plethora of other best practices that would limit how much work in done in a constructor. Most of these can be summed up in SOLID principles, and those same principles would not prevent you from doing work in the constructor.

PS. I feel obligated to say that while I assert here that there is nothing wrong with doing some work in the constructor, it's not the place to be doing a bunch of work either. SRP would suggest that a constructor should do just enough work to make it valid. If your constructor has too many lines of code (highly subjective I know) then it's probably violating this principle, and could probably be broken into smaller better defined methods and objects.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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