25

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..

Should a constructor be firing off all that work, or something more like the following?

$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?

0

5 Answers 5

57

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.

1
  • 7
    God spoke directly to you, feel blessed. Robert C. Martin replied your answer :O Jul 31, 2017 at 17:12
4

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.

4

The problem, from an OO design perspective, is not so much whether that functionality should be implemented in a constructor (as opposed to within other methods on that class), but whether the SupportTicket class should know how to do all of those things.

In a nutshell, the SupportTicket class should be modelling a support ticket, and only a support ticket. Creating an email, knowing how to send that email to the customer, queueing the ticket for processing, etc. are all lumps of functionality that you should move from your SupportTicket class and encapsulate elsewhere. The benfits of doing this include lower coupling, higher cohesion, and improved testability.

Have a look at the Single Responsibility Principle, which explains the benefits of doing this. In particular, this blog is a good place to read up on SRP and the other keystone principles of good OO design.

1
  • this hits the nail on the head. Whether you use a factory (as other posts mention) or not is irrelevant.
    – Egwor
    Mar 7, 2009 at 14:06
2

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? (Screechingly Obvious Code)

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 parameters...)
    

    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 two calls from the client code, where the mental thought is 1 thing.

    $ticket = new SupportTicket($customer, $title);
    
    $ticket -> handleNewSupportTicket(...other parameters...)
    
  • 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, i.e., 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 parameters...)
    

    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 fourth 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 :(.

2

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." The same applies here for the same reason.

The longer answer will have to wait, but see "Screetchingly Obvious Code".

1

Your Answer

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