Loose coupling vs Encapsulation. Best approach for a balanced design

Question

According to the next examples:

class InvoiceGenerator
{
   function create(Invoice $invoice)
   {
      $invoice->create();
   }
}



class InvoiceGenerator
{
   function create($invoiceData)
   {
      $invoice = new Invoice();
      $invoice->create($invoiceData);
   }
}

The first example has less coupling between the InvoiceGenerator and Invoice classes, because InvoiceGenerator does not require the Invoice class. Plus, it could handle not only a class, but a whole interface with very little modification. I've read many times this principle that says: code to interface, not implementation. The downside of this case is that I'm forced to instantiate the Invoice class in the client code.

The second one has more encapsulation. All the process of instantiation and creation of an invoice is delegated to the InvoiceGenerator class. Even though both classes are coupled, this makes sense because an "invoice generator" wouldn't do anything without invoices.

Which would you consider most appropriate? Or what are the key points for a balanced design between both of them?

Answer 1

First of all, I think you are mixing up some things. It looks like your InvoiceGenerator class is a factory class. Its responsibility is to create and return an Invoice object. The Invoice object is not a dependency of the InvoiceGenerator (in which case it would indeed be better to pass the Invoice object as an argument, known as Dependency Injection).

However, as I said, the InvoiceGenerator class seems to be a factory class, and the whole idea of a factory class is that the logic for instantiating an object is encapsulated within that class (as in your second example). If you would pass the Invoice object as an argument (as in your first example), you would have to instantiate it somewhere else, and if you have multiple places in your code where this happens, you would end up duplicating the instantiating logic. The whole idea of the factory pattern is to prevent this.

By using a factory class you can encapsulate the object instantiation logic and avoid duplication. You can also put some more advanced logic into the InvoiceGenerator's create() method to decide what type of object you want to return. For instance, you might want to return an Invoice object if the total price is > 0, but a CreditNote object if the price is < 0. Of course they should both extend the same interface (for instance InvoiceInterface).

Also, it looks like you are delegating the actual initializing of the Invoice object to the object itself, since the create() method of InvoiceGenerator does nothing more than calling the create() method of the Invoice object, where the actual logic seems to take place. This violates the single responsibility principle, because the Invoice class is now both responsible for holding data about an invoice and setting all the data from an array. The latter should instead be the responsibility of the factory class.

So, I would create the class like this:

class InvoiceFactory
{
    public function create(array $data)
    {
        if ($data['total'] >= 0) {
            $invoice = new Invoice();
        } else {
            $invoice = new CreditNote();
        }

        $invoice->setTotal($data['total']);
        // set other data using setters as well

        return $invoice;
    }
}

As you can see, there is no create() method being called on the $invoice object.