CarlBorgas\Blog

Is Logging a (Single) Responsibility?

Is Logging a (Single) Responsibility?

Carl Borgas's photo
Carl Borgas
·

9 min read

Perhaps the most open-to-interpretation Principle of the S.O.L.I.D 1 Principles is the Single-Responsibility Principle 2. This priciniple is quoted as being about people 3. More specifically,

When you write a software module, you want to make sure that when changes are requested, those changes can only originate from a single person, or rather, a single tightly coupled group of people representing a single narrowly defined business function. 4

What about logging? Is logging a responsibility and does logging originate from a single person? This person being us, the engineers or developers.

From my experience, I would argue yes. Logging is a responsibility.

Logging, Logging Everywhere

Let's look at some code. If you work with any Object-Oriented language or largely Object-Oriented codebase you might be familiar with this style of code you're about to see. Below is a snippet of code I shared with some colleagues of mine. I asked them. What is this code doing?

Try to come up with a brief description of what this code is doing.

class FrontDesk
{
    public function __construct(
        private CustomerRepository $customerRepository,
        private WelcomeMessageSender $welcomeMessageSender
    ) {}

    public function addMemberships(array $emailAddresses): void
    {
        $this->logger->debug(sprintf('Adding memberships for %s', implode(', ', $emailAddresses)));
        $customers = $this->customerRepository->getCustomersByEmailAddresses(
            $emailAddresses
        );

        $this->logger->debug(sprintf('Found %s customers', count($customers)));

        $previousCustomers = [];
        $newCustomers = [];

        foreach ($customers as $customer) {
            if ($this->hadMembership($customer)) {
                $this->logger->debug(sprintf('Customer %s had membership', $customer->getName()));
                $previousCustomers[] = $customer;
            } else {
                $this->logger->debug(sprintf('Cusomer %s has never had a membership', $customer->getName()));
                $newCustomers[] = $customer;
            }
        }

        foreach ($newCustomers as $customer) {
            $this->logger->debug(sprintf('Adding membership for %s', $customer->getName()));
            try {
                $this->addMembership($customer);
                $this->logger->debug(sprintf('Membership added for %s', $customer->getName()));
                $this->welcomeMessageSender->sendWelcomeMessage($customer);
            } catch (RuntimeException $exception) {
                $this->logger->error(
                    sprintf( 'Failed to add membership for %s', $customer->getName()),
                    ['exception' => $exception]
                );
                throw $exception;
            }
        }

        foreach ($previousCustomers as $customer) {
            $this->logger->debug(sprintf('Renewing membership for %s', $customer->getName()));
            $this->renewMembership($customer);
            $this->welcomeMessageSender->sendWelcomeBackMessage($customer);
        }
    }

    // ...
}

So, what is this code doing? You might have said it finds customers based on their email addresses and it will add or renew memberships for an array of customer email addresses. Or you might have been more granular and said it finds previous members and non-members, and so on.

Despite me priming your mind with the thought of logging you may have done exactly what my colleagues did and (subconsciously) completely ignored it. If you were introspective you might have even thought "this logging makes this hard to read".

To me, this is a violation of the single-responsibility principle. I'm going to show you that you can remove all logging from this class without losing that functionality.

Which, will result in something like this.

    // ...

    public function addMemberships(array $emailAddresses): void
    {
        $customers = $this->getCustomers($emailAddresses);
        $previousCustomers = [];
        $newCustomers = [];

        foreach ($customers as $customer) {
            if ($this->hadMembership($customer)) {
                $previousCustomers[] = $customer;
            } else {
                $newCustomers[] = $customer;
            }
        }

        foreach ($newCustomers as $customer) {
            $this->addMembership($customer);
            $this->welcomeMessageSender->sendWelcomeMessage($customer);
        }

        foreach ($previousCustomers as $customer) {
            $this->renewMembership($customer);
            $this->welcomeMessageSender->sendWelcomeBackMessage($customer);
        }
    }

This is undoubtedly much clearer. It also makes it more obvious as to how we can clean it up even further (you've probably already thought of it).

Logging, but somewhere else

How do we fix this? How do we separate the responsibility of logging from the other violations of the single-responsibility principle (because there are plenty)?

One very simple approach would be to move the logging to the private or the protected methods within the same class. Although a small win, we would still have the same problem, too many responsibilities and strong coupling of those responsibilities.

There are many very viable solutions to this problem, each with its own merits. I'm going to show you two of them.

Extract Interface & Extend Class

If you're not familiar with Extract Interface then have a look over at Refactoring Guru 5.

It is in combining these simple concepts that we have perhaps the easiest solution to our problem.

The full refactor can be viewed in my gist here. Take particular notice of the FrontDesk class and its lack of logging. I'll break it down step-by-step.

1. Extract the public methods of the FrontDesk class to an interface.
interface FrontDeskInterface
{
    public function addMemberships(array $emailAddresses): void;
}
2. Implement that interface in the FrontDesk class.
- class FrontDesk
+ class FrontDesk implements FrontDeskInterface
3. Change any private methods that we call in the FrontDesk class protected so that we can call them from any child class.
-    private function addMembership(Customer $customer): void
+    protected function addMembership(Customer $customer): void
4. Create an aptly named LoggingFrontDesk which extends our FrontDesk class and implements our new interface.
class LoggingFrontDesk extends FrontDesk implements FrontDeskInterface
{
    public function __construct(
        CustomerRepository $customerRepository,
        WelcomeMessageSender $welcomeMessageSender,
        private LoggerInterface $logger
    ) {
        parent::__construct($customerRepository, $welcomeMessageSender);
    }

    public function addMemberships(array $emailAddresses): void
    {
        // to be implemented
    }
}
5. Override the protected methods of the FrontDesk class in the LoggingFrontDesk class and call the parent methods (of the same name) from within them (your IntelliJ IDE can do this for you).
class LoggingFrontDesk extends FrontDesk implements FrontDeskInterface
{
    public function __construct(
        CustomerRepository $customerRepository,
        WelcomeMessageSender $welcomeMessageSender,
        private LoggerInterface $logger
    ) {
        parent::__construct($customerRepository, $welcomeMessageSender);
    }

    public function addMemberships(array $emailAddresses): void
    {
        parent::addMemberships($emailAddresses);
    }

    protected function addMembership(Customer $customer): void
    {
        parent::addMembership($customer);
    }

    protected function renewMembership(Customer $customer): void
    {
         parent::renewMembership($customer);
    }

    protected function getCustomers(array $emailAddresses): array
    {
        return parent::getCustomers($emailAddresses);
    }
}
6. Move all the logging from the original FrontDesk to the new LoggingFrontDesk.

Best viewed in my gist here, but here is a partial example.

class LoggingFrontDesk extends FrontDesk implements FrontDeskInterface
{
    // ...

    protected function getCustomers(array $emailAddresses): array
    {
        $customers = parent::getCustomers($emailAddresses);
        $this->logger->debug(sprintf('Found %s customers', count($customers)));
        return $customers;
    }

    // ...
}

What we have now are separated responsibilities, logging, and all that other stuff. Because we have implemented the same interface, anywhere that we had previously had a dependency on the FrontDesk class we can now refactor to use our interface and substitute with the LoggingFrontDesk or leave the choice up to any clients.

7. And don't forget to use it

Here I can use the FrontDesk or LoggingFrontDesk

class SomeOtherClient
{
    public function __construct(
-        private FrontDesk $frontDesk
+        private FrontDeskInterface $frontDesk
    ) {}

    // ...
}

Intermission Quiz!

Question: What is the LoggingFrontDesk class responsible for?

Answer: You guessed it, logging.

Extract Methods, Extract Interface & Wrap Class

But what if I don't like that?. Perhaps in this scenario you would prefer composition over inheritance 7. Then let me pose an alternative.

You might think that overriding parent methods (unless when writing unit tests around refactored code) might signal some other design flaw and you would probably be correct. To avoid this we can use Wrap Class. Wrap Class is (again, in my opinion) a more descriptive name for the Decorator Pattern. If you're not familiar with that then the Refactoring Guru 6 has your back.

We'll also pair this with Extract Interface and Extract Method. If you're not familiar with the refactoring technique Extract Method then take a look at the JetBrains Help 8 article.

This approach comes with a significant caveat that should be considered. Particularly with a scenario like our FrontDesk which relies on private members. In this design, I have made the decision to extract the private members to a new class. This decision is not to be made lightly when refactoring code. Consider what it is that the private members are doing and whether or not their behavior of responsibility can safely be moved. Assuming it can, let's take a look at what that looks like.

Again, best viewed in my gist here but I'll break this down too.

1. Extract the public methods of the FrontDesk class to an interface. Again, I went with FrontDeskInterface.
2. Create another aptly named LoggingFrontDesk which implements our new FrontDeskInterface and accepts a FrontDesk and a Logging interface as a dependency.
class LoggingFrontDesk implements FrontDeskInterface
{
    public function __construct(
        private FrontDesk $frontDesk,
        private LoggerInterface $logger
    ) {}

    public function addMemberships(array $emailAddresses): void
    {
        // to be implemented
    }
}
3. Here is where we will also separate the responsibilities of managing memberships for customers from the responsibility of associating emails with customers and determining the status of a customer.

Extract the private members (methods and dependant properties) to public methods in a new class (I went with MembershipManager).

class MembershipManager
{
    /**
      * @throws RuntimeException
      */
    public function addMembership(Customer $customer): void
    {
        // ...
    }

    public function renewMembership(Customer $customer): void
    {
        // ...
    }

    public function hadMembership(Customer $customer): bool
    {
        // ...
    }
}
4. Accept that new class as a dependency in our FrontDesk class and replace the internal calls to the private methods with calls to the public methods of that dependant class.
class FrontDesk implements FrontDeskInterface
{
    public function __construct(
        protected CustomerRepository $customerRepository,
        protected WelcomeMessageSender $welcomeMessageSender,
+       protected MembershipManager $membershipManager
    ) {}

    public function addMemberships(array $emailAddresses): void
    {
        $customers = $this->customerRepository->getCustomersByEmailAddresses(
            $emailAddresses
        );

        $previousCustomers = [];
        $newCustomers = [];

        foreach ($customers as $customer) {
-           if ($this->hadMembership($customer)) {
+           if ($this->membershipManager->hadMembership($customer)) {
                $previousCustomers[] = $customer;
            } else {
                $newCustomers[] = $customer;
            }
        }

        foreach ($newCustomers as $customer) {
-           $this->addMembership($customer);
+           $this->membershipManager->addMembership($customer);
            $this->welcomeMessageSender->sendWelcomeMessage($customer);
        }

        foreach ($previousCustomers as $customer) {
-           $this->renewMembership($customer);
+           $this->membershipManager->renewMembership($customer);
            $this->welcomeMessageSender->sendWelcomeBackMessage($customer);
        }
    }
}
5. In our LoggingFrontDesk class, "wrap" the public method of our FrontDesk class and implement logging.
class LoggingFrontDesk implements FrontDeskInterface
{
    public function __construct(
        private FrontDesk $frontDesk,
        private LoggerInterface $logger
    ) {}

    public function addMemberships(array $emailAddresses): void
    {
        $this->logger->debug(sprintf('Adding memberships for %s', implode(', ', $emailAddresses)));
        $this->frontDesk->addMemberships($emailAddresses);
    }
}
6. Repeat the process of extracting MembershipManager class to a new interface and wrapping it in a logging "version" of that class. I created a MembershipManagerInterface and a LoggingMembershipManager.

Again, best viewed in the gist here, but here is another partial example.

class LoggingMemembershipManager implements MembershipManagerInterface
{
    public function __construct(
        private MembershipManagerInterface $membershipManager,
        private LoggerInterface $logger
    ) {}

    public function addMembership(Customer $customer): void
    {
        $this->logger->debug(sprintf('Adding membership for %s', $customer->getName()));
        try {
            $this->membershipManager->addMembership($customer);
            $this->logger->debug(sprintf('Membership added for %s', $customer->getName()));
        } catch (RuntimeException $exception) {
            $this->logger->error(
                sprintf('Failed to add membership for %s', $customer->getName()),
                ['exception' => $exception]
            );
        }
    }

    // ...
}

Despite a large number of changes, this is perhaps the best design.

  • The responsibility of managing membership has been moved to its own class.
  • The responsibility of logging has been moved to its own class.
  • We have decoupled these responsibilities.
  • We have created reusable and more easily testable components.

Is it worth it?

The most devout S.O.L.I.D Principle Disciples might respond with an unequivocal Yes. But perhaps this introduces more layers than we need or than we will even care for. If your codebase is littered with logging which you find interrupting then it may well be worth considering either option.

What is important to note with these strategies is that it costs a small amount of effort to make this change earning us quite a large reward.

refactoringloggingPHPObject Oriented ProgrammingSOLID principles