How to refactor code so that a facade class could be decoratable?

Question

I've got a class that is a facade class (encapsulates complex-ish behaviour for reusability). It has a function called manage (the class is called Manager):

function manage()
{
    $entityBuilder = 'Some builder';
    $someData = 'Some data';
    foreach ($someData as $someDatum) {
        if (!$entityBuilder->doesNeedBuilding($someDatum)) {
            continue;
        }

        if (!$entityBuilder->canBuild($someDatum)) {
            continue;
        }

        try {
            $entityBuilder->build($someDatum);
        } catch (Exception $e) {
        }
    }
}

I would like to log the result to another table something like this:

function manage()
{
    $entityBuilder = 'Some builder';
    $someData = 'Some data';
    foreach ($someData as $someDatum) {
        if (!$entityBuilder->doesNeedBuilding($someDatum)) {
            // Log status 'Skipped'.
            continue;
        }

        if (!$entityBuilder->canBuild($someDatum)) {
            // Log status 'Failed'.
            continue;
        }

        try {
            $entityBuilder->build($someDatum);
            // Log status 'Success'.
        } catch (Exception $e) {
            // Log status 'Failed'.
        }
    }
}

But of course it convolutes the function; and even it has a very vague name, logging clearly breaks SRP.

I usually use decorators for these kinds of stuff, but since this is a facade and since it encapsulates a number of steps, I can't simply use a decorator here.

I also thought about returning a list of statuses for each operation and then log somewhere else, but it also is some sort of convolution.

Are there any good architects around that can guide me on solving this one?

Answer 1

Logging would only break the SRP if the manager class would implement the log mechanics by itself. But adding some event mechanics to send notifications to a callback does not, for example

 $notify = // initialize this member variable by a function, passed to the constructor
 // ...
 foreach ($someData as $someDatum) {
      if (!$entityBuilder->doesNeedBuilding($someDatum)) {
          $notify('Skipped');
          continue;
      }

Now, you can initialize $notify with a default value of a "do nothing" function, and with a real logging function whereever you like (for example, in a decorator, which provides the logging implementation).

And yes, that makes the code slightly more complex, but does this convolute the code? In my opinion, this is perfectly acceptable for the benefit one gets.

Answer 2

First, I'd just like to say, returning a list of statuses could potentially be a nice solution (I think functional-oriented people would prefer that), but of course, it may or may not be easy to implement, depending on what's going on in the rest of your system.

But if you want to go the decorator route, you could decorate your $entityBuilder:

class LoggingBuilder implements EntityBuilder
{
    public function __construct(EntityBuilder $builder) {
        $this->wrappedBuilder = $builder;
    }

    public function doesNeedBuilding($someDatum)
    {
        if (!$this->wrappedBuilder->doesNeedBuilding($someDatum)) {
            // Log status 'Does not need building'.
        }
    }

    public function doesNeedBuilding($someDatum)
    {
        if (!$this->wrappedBuilder->canBuild($someDatum)) {
            // Log status 'Cannot build'.
        }
    }

    public function doesNeedBuilding($someDatum)
    {
        try {
            $this->wrappedBuilder->build($someDatum);
            // Log status 'Success'.
        } catch (Exception $e) {
            // Log status 'Failed'.
        }
    }
}

And then do:

function manage()
{
    $entityBuilder = new LoggingBuilder(/* someBuilder */);
    foreach ($someData as $someDatum) {
        if ($entityBuilder->doesNeedBuilding($someDatum) &&
            $entityBuilder->canBuild($someDatum)) 
        {
            $entityBuilder->build($someDatum)
        }
}

That's the basic idea, but you could tinker with it in various ways. E.g., consider what happens if you don't want to use the wrapper (say, in a different part of the application, or in a test suite). If build throws an exception, what should handle it? Maybe the doesNeedBuilding wrapper should re-throw, and the entire if-statement in manage should be wrapped in a try-catch to gracefully handle failure, even if there's no logging. Things like that. Try to keep it simple, though.

P.S. PHP is not something I use every day, so if there's anything that looks odd to a PHP dev, don't get confused by it.