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.

Answer 2

If you want to follow SOLID , then the first option is the closest to meet the requirements.

The first option, InvoiceGenerator class with one responsibility to create an invoice and does not instantiate an invoice object. The invoice object can be replaced (extension) with a sub-class or a class that implements the invoice interface.

The second option, the invoice object is hardcoded and is not open for extension. It is open for modification if the implementation of the invoice class changed in the future. Also, it is not possible to test (PHPUnit) the InvoiceGenerator with mock instance of the invoice class.

In my opinon, choose what suit your application needs while considering who is going to use and maintain your code.

An option to make the InvoiceGenerator class less coupled and you don't have to instantiate the invoice object in the client code but you give the option for the client code to set its own instance of the invoice object.

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

Answer 3

You shoud consider approach the problem with a Factory Method Design Pattern something like:

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

Answer 4

This is actually a difficult design problem given php as the language.

I do prefer less coupling rather than encapsulation (DRY dont repeat yourself).

For myself, I have built invoice software this is generally how I have it Structured.

Inputs: Hours Worked (Hourly) Items Sold (Items) Goes into a InvoiceGenerator which outputs an Invoice.

class Hourly {
}

class Items {
}

Class InvoiceGenerator {
    //Returns Invoice
    //A list of hourly objects and items
    function createInvoice($state, $hourly = array(), $items = array() {
        $invoice =  new Invoice($hourly, $items);
        $invoice->applyTaxes($state):

        //Do Other Things

        return $invoice;
    }
}

The key points in all my projects are #1 Readabilioty and #2 Not Repeating yourself By encapsulating your data, you make it more difficult to test and its less flexible.

Array data can be thrown around in PHP like an object but you miss the ability to have other functions on the data. Like for instance if 1 object is TAX Exempt. You now have to add to your massive array structure the ability to check for that.

Or with the less coupled approach you just have a method that returns the tax value (0 or otherwise).

I hope that example shows why a less coupled approach is often the better one.