business logic in constructors a good idea?

Question

I'm currently rebuilding a specialised ticket system at work (mainly used to support people with faults in remote sensing hardware...). Anyway, I was wondering whether doing lots of workflow type activity in an object's constructor is a good idea.

For example, there is currently this:

$ticket = new SupportTicket($customer, $title, $start_ticket_now, $mail_customer);

as soon as the object is created, it'll put a row into a database, go and mail the customer a confirmation e-mail, possibly send a text message to the nearest technician etc etc etc.

My question is, should a constructor be firing off all that work, or something more like:

$ticket = new SupportTicket($customer, $title);

$customer->confirmTicketMailed($ticket);

$helpdesk->alertNewTicket($ticket);

If it helps, the objects are all based on the ActiveRecord style.

I guess it may be a matter of opinion, but what do you think is the best thing to do?

Answer 1

Split things up. Really, you don't want a ticket to know how it should send e-mail etc. That is the work of a service like class.

[Update] I don't think the suggested factory patterns are good for this. A factory is useful if you want to create different implementations of tickets without putting this logic in the ticket itself (via overloaded constructors for instance).

Take a look at the Service concept proposed in Domain-Driven Design.

Services: When an operation does not conceptually belong to any object. Following the natural contours of the problem, you can implement these operations in services. The Service concept is called "Pure Fabrication" in GRASP.

Answer 2

If the constructor does all that work then the constructor knows about many other domain objects. This creates a dependency problem. Should the ticket really know about the Customer and the HelpDesk? When new features are added, isn't it likely that new domain objects will be added to the workflow, and doesn't that mean that our poor ticket will have to know about an ever increasing population of domain objects?

The problem with spiderwebs of dependency like this is that a source code change to any one of the domain object will have an impact upon our poor ticket. The ticket will have so much knowledge of the system that no matter what happens, the ticket will be involved. You will find nasty if statements gathering inside that constructor, checking the configuration database, and the session state, and so many other things. The ticket will grow to become a god class.

Another reason I dislike constructors that do things is that it makes the objects around them very hard to test. I like to write lots of mock objects. When I write a test against the customer I want to pass it a mocked out ticket. If the constructor of ticket controls the workflow, and the dance between customer and other domain objects, then it is unlikely that I will be able to mock it out to test the customer.

I suggest you read The SOLID Principles, a paper I wrote several years back about managing dependencies in object oriented designs.

Answer 3

Another nice feature of a factory method is that you can give it a meaningful name, unlike a constructor

Answer 4

the short answer is No. In hardware design, we used to have a saying,"Don't put a gate on the clock or the reset line - it obscures the logic." Same applies here for the same reason. Longer answer will have to wait, but see "ScreetchinglyObviousCode". Alistair

Answer 5

Slightly longer answer, now that I have a keyboard (not the iPwn). ... Actually I've never been happy with any of the available answers, but let's look at them. Choices are built around two evaluation questions:

E1. Where does the knowledge belong of the business logic?

E2. Where will the next reader of the code look? (ScreechinglyObviousCode)

some choices:

• In the client code (the object that does "new SupportTicket"). It probably doesn't / shouldn't know the business logic, evidently, or else you wouldn't be wanting to create that fancy constructor. If it IS the right place for the business logic, then it should say:

  $ticket = new SupportTicket($customer, $title);

  handleNewSupportTicket($ticket, ...other params...)

where, in order to protect E2, "handlenewSupportTicket" is the place where that business logic is defined (and the next programmer can easily find it).

• In the support ticket object, as a separate business call. Personally, I'm not really happy with this, because it's 2 calls from the client code, where the mental thought is 1 thing.

  $ticket = new SupportTicket($customer, $title);

  $ticket -> handleNewSupportTicket(...other params...)

• In the class of support tickets. Here it is expected that the business logic resides in the Support Ticket area, but since new support tickets absolutely need to be handled immediately and not later, this important task can't be left to anyone's imagination or mistyping, ie, specifically not to the client code. I only know how to code class methods in Smalltalk, but I'll take a stab at pseudo-code:

  $ticket = SupportTicket.createAndHandleNewSupportTicket(...other params...)

assuming that the client code needs the handle to the new ticket for other purposes (otherwise the "$ticket =" would vanish.

I'm not awfully fond of this, because other programmers don't find it so natural to look for business logic in class or static methods. But it's the third choice.

• The only 4th choice is if there is another place the business logic happily resides and other programmers will naturally look for it, in which case it goes into a class/static function there.

  $ticket = SupportTicketBusinessLogic.createAndHandleNewSupportTicket(...other params...)

where that class/static function does the multiple calls needed. (but now we once again have the possibility that tickets can be constructed and not handled properly :(.

Alistair