0

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?

1
  • 2
    Logging is an orthogonal concept; it does not break SRP because it is not considered core functionality of the class. If logging broke SRP, we would never do logging. Apr 14 at 11:52

2 Answers 2

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.

4
  • Thank you for your answer. Adding an event dispatcher is a nice solution. I like it. I just would like to clarify one bit: how would you use a decorator in this case? That notify function should become a part of the public interface in this case, right? Or you mean that I can just dispatch an event and subscribe to it from somewhere else without bothering about decorators at all?
    – pro100tom
    Apr 14 at 17:40
  • 1
    @pro100tom - what Doc Brown is suggesting is to pass in an external function (as a constructor parameter) and store it as a callback (as a member variable called $notify). So a function that is not part of this class (a function supplied from elsewhere by the caller and/or creator of this class) plays the role of the "notify" function within this class. Excluding the constructor, it doesn't have to be a part of the public interface ($notify doesn't have to appear publicly to other code, except as a constructor parameter). Apr 14 at 18:14
  • Thank you for explaining. It doesn't seem flexible because only one function can be passed; also it can be overridden. I think I will stick with dispatching the event.
    – pro100tom
    Apr 14 at 18:16
  • 2
    @pro100tom - you can always pass in multiple functions, or an object with several different functions, but it's OK to go with events, IMO. Dispatching an event is essentially the same as this (it's the same underlying mechanism, subscribing to an event is like passing in a callback, triggering it is like calling that callback), so the flexibility is more or less the same. It's a choice between a more use-case specific use of the Observer pattern, and the more general-purpose event system. Apr 14 at 19:27
0

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.

1
  • Thank you for your response. The problem with your decorator approach is that when using the manager it is not known whether the entityBuilder is used in there or not; it is an implementation detail. Therefore using the statusList sound like a way to go. However it gets complicated because I also need to "reveal" the just built entity and it doesn't belong to a statusList variable. It means that the returned result must be a more complex object.
    – pro100tom
    Apr 14 at 17:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.