Does logging inside a class violate the SRP?

Question

I wrote a class that takes a Logger class as one of its arguments:

class QueryHandler:

    def __init__(self, query: Query, logger: Logger) -> None:
        self.query = query
        self.logger = logger

    def run(self) -> None:

        try:
            r = requests.get(self.query.url)
        except requests.exceptions.RequestException as err:
            raise QueryException(f'Failed to complete {self.query.id}') from err  # log outside in caller
        else:
            sc = r.status_code
            if sc != 200:
                self.logger.warning(f'{self.query.id} returned with code: {sc}')
            else:
                self.query.response = bytearray(r.content)

There is other functionality, but a colleague, specifically focused on this method, cites that, because the class calls the self.logger.warning, it violates the single responsibility principle (SRP). My position was that the class is responsible for handling a particular query object, and when run is called, by calling the self.logger.warning it is not a violate of the SRP because the class it not concerned with how the logging takes place, that is delegated to the Logger class. I further argued that the SRP doesn't apply to functions, and thereby methods (i.e., bound functions).

If the run method implemented the logging logic (opening a file, formatting the message, etc.), then I would agree with my colleague.

Is my understanding of the SRP incorrect?

Answer 1

Your colleague is taking the word "Single" too literally and dogmatically.

SRP just means a class should have a single conceptual purpose. What constitutes a single conceptual purpose will differ between different types of software, and how the module fits in a larger program. There isn't some sort of mechanical definition you can test against in a vacuum, it's a judgement call for humans to decide.

When you see a class called File, you should be able to expect it to only deal with files. If you see a class called Random you should expect it to only be for randomization purposes. You wouldn't expect it also to write to Files, or open network connections, or parse Urls or something.

Any class that does anything useful is going to do multiple things if you treat "responsibility" too simply. Any if statement will result in two possible code paths, thus anything with it will be doing at least two things, but an entire program without any conditionals isn't going to be terribly useful for many applications.

Including logging in a class is normally not sensibly considered a violation of SRP. In an example of File, Random, or ExpensiveMachineController class, I would not expect the logging behavior to have any influence over how those classes behave, with regards to how I'm hooking them up to other classes. Logging would provide me a diagnostic function, and I should be able to ignore that completely and still use those classes correct.

Sure, there could be bugs and strange edge cases could cause weirdness, but avoiding that is a big part of the craft of software development, and the design of the logging should prevent that.

Answer 2

People have gone mad thinking about what constitutes a "single responsibility". The principle is sound, but it is usually stated in a way which cause more confusion than clarity.

The principle is about seperating independent concerns - independent at the level of requirements and stakeholders. In other words, any single thing a product owner could want you to change about the application without changing anything else.

The product owner probably don't care about your logs at all, so this is more of an implementation detail. And in any case, the logging is not independent of the querying - it is directly depending on the result of the query. So I don't see a SRP violation.

A SRP violation would be for example if the same module performed a query and formatted the result into a CSV file and mailed it it to the financial department. You can imagine the PO would want to change this to an Excel file instead, without changing the query. So the implementation of the CSV formatting and the query execution should not be in the same class.

Answer 3

There are already two excellent answers here that I fully support. I’d nevertheless like to add a couple of thoughts for the sake of completeness:

• The name “single responsibility principle” is utterly misleading. Many misunderstand it as being about the responsibility of the class. But Uncle Bob, who invented the term, explained that it is not about responsibility of the class, but about a single “reason to change”:

  Gather together the things that change for the same reasons. Separate those things that change for different reasons.

• Using this alternative and more accurate description, you’ll quickly find out for very simple cases, that "a single reason to change” is very ambiguous in theory and very difficult to apply in practice. For example, any parameter used in a method could refer to a changing interface, which could then be interpreted as an additional reason to change on top of the changes related to the prime purpose.

• Moreover, let’s take the arguments of the SRP extremists by the book: very often different methods correspond to different responsibilities (it's all a question of granularity at which you consider the responsibility). You’d end up by breaking down your classes into many classes having only a single method. Without prejudice to the extra complexity of such a strange design, it seems at the antipode of the OOP philosophy of proper encapsulation. This shows that SRP makes no sense if applied too narrowly.

• Responsibility means different things to different people. In the end, what truly matters is the separation of concerns. And if a class is about handling queries, logging that handling still belongs to the same concern, provided that the logging mechanism is not reimplemented in the class, but relies on a logger class that encapsulate the log management concern.

With these arguments, you come to the same conclusion as JacquesB and Wathsisname: you need to think about your design rather than blindly following an ambiguous principle, especially considering the risk of overengineered design.

Answer 4

Actually yes, if you look strictly, the logging logic should not be a concern of a method (function) doing other things. But, then it can be applied to any method/function, so where would you put the logging statement? In the end most code you can see has logging statements somewhere in the code. Having said that, this kind of concern is called a cross cutting concern, since they are not directly part of the concern of the given code (method/function) but cannot be cleanly decomposed.

A solution that addresses that issue is called aspects. I am not saying that you must do it that way, but at least you can see how one more or less clean (concern separating) solution may look.

See also: https://en.wikipedia.org/wiki/Cross-cutting_concern and https://en.wikipedia.org/wiki/Aspect-oriented_programming

Edit:

Since I see the confusion, I'll put a little explanation how logging should work with AOP.

Using concept of SRP your class should do one thing. Same goes for each method of class, they should do one task. At this point we all could agree that any logs put in should relate context known only to the class. You can design methods in your class, so that they are short and concise (as a matter of fact that should be preferred way of constructing classes and methods). So based on that you could say that anything you may wish to put in your log can be constructed from methods you call and state the class is in (i.e. field values). And these are exactly the things aspects are operating with.

Answer 5

"Principles" in software are concepts to assist you, and make your live easier. Whenever someone argues based on a "principle" that you should make your life harder, either that someone misunderstood the principle*, or - more likely - there's a communication failure between the two of you, meaning you misunderstand what they are saying, they misunderstand the problem, or both.

The solution is to reframe the discussion to look from a different perspective. That usually is sufficient to at least highlight, if not resolve, the misunderstanding.

You were discussing a specific issue ("should we log like this"). Upon asking your colleague why it's an issue, you heard a response that didn't make sense. Assume failed communication. Reframe - you can freely admit that you don't follow the arguments and therefore want to look from a different angle. Ask for proposed improvements, and what the specific benefits of the proposed improvements are. "It follows so-and-so principle" is never a benefit on it's own. Principles are rules of thumb that guide you on a path that offers benefits, but unless you can actually spot and exploit these benefits, you're in a cargo cult.

Similarly, when you notice a communication failure while discussing proposed improvements, you can reframe by asking about the problem the improvements are intended to solve.

---

Is my understanding of the SRP incorrect?

*If your understanding of the discussion as presented in the question is 100% correct, and matches with your colleague's understanding of that discussion, then you're right, they are wrong, and you'll have plenty of annoying interactions ahead of you.

But it's perfectly possible that your colleague was talking about a different, legitimate, concern and simply used SRP as one argument, or invoked the principle due to an inability to articulate the actual concern. I wasn't there and have no idea what was said, but @Tommy Andersen provided one possible example in a comment:

SRP is not the issue here. The problem I see with the logging, is that you do not offer the result that caused the log message, back to the caller. There is no way for the caller to know what status code the query resulted in, and therefore whether the response was correctly retrieved. On the other hand, had you offered the status code back to the caller I would see no reason to have the logger there in the first place. I would then move it outside the function.

Answer 6

Well, you are both a bit right. SRP can certainly be applied to functions, and in fact I think it applies to functions even more than it does to classes.

OTOH, logging isn’t a responsibility of the function. You are logging for debugging purposes. Notionally that means it doesn’t even exist as far as the class is concerned. But if it was a responsibility of the class, the actions would be bound together as a single action and responsibility. In that case, the function would be doing one thing, making a request and writing the response to one of two places. The fact that there are two places, and that to do one of them required an object to be injected is irrelevant.

Answer 7

It is mostly clear from your code example that you/we are talking about debug logging, or logging for debug purposes or reasons. I think other answers already tackle the issue. Debug logging is like a separate "layer".

However, for completeness' sake(1), there is also transaction logging and/or audit logging [not sure if that's the official name], where a log is kept for business reasons.

Quite possibly, in that case you might want to separate this logging from the other functionality of the class. For example, with a logging wrapper, or perhaps reifying operations/transactions into classes plus some run-then-log code, and so on.

(1) It's not explicit in the title, so this may be useful to other people having the same question but for other kinds of logging.

Answer 8

SRP taken to its extreme isn't practical. Let's take a practical, familiar, real-life example where SRP has been successfully implemented: fast food restaurant chains. In SRP, you would define a restaurant in a chain to have a single responsibility of serving food to customers. However, implementation details include things like handling credit card payments, obtaining ingredients, following food safety regulations, etc.

You won't see a bank inside the restaurant, nor a farm just outside to obtain the potatoes, cows, chickens, etc, plus the food to feed the livestock, nor a processing plant to turn those ingredients into their final products. The restaurant has several outside sources it depends on for most of what it needs serve its single responsibility.

It's worth noting that by doing this, a restaurant chain can change, for example, the credit cards it supports, or the style of French fries it serves, simply by modifying the relevant service that all the restaurants in the chain rely on. The individual restaurants largely don't need to know, or care, how those external services work, and are largely not directly impacted by implementation changes made upstream.

Likewise, your QueryHandler needs to log certain data, such as errors. It's reasonable to pass in a Logger to your implementation in order to facilitate this function, just as the restaurant needs a way to handle credit cards. Because of how this code is designed, it only needs to worry about handling queries, and doesn't need to worry about how logging actually happens. A developer can change the Logger to a memory logger, a network logger, or even a blackhole logger (e.g. write to /dev/null), and the QueryHandler doesn't need to know that anything changed. It's handling its single responsibility.

In a practical implementation of SRP, you need to think about what the actual steps are in order to fulfill a responsibility. You can't reasonably implement logging inside QueryHandler, because then you'd have a fixed implementation here, and presumably every other class that needs logging, which would be a nightmare if you ever needed to change how logging operates.

The Logger's responsibility is to provide a simple logging API, and the QueryHandler's responsibility is to use that Logger to log errors in the course of handling queries. Even though QueryHandler is doing two "things" at a technical level, it is handling just one responsibility, and that is to reliably service queries. That reliability needs a Logger, so that makes it part of QueryHandler's responsibility. A class may do many "things" internally, but from an external point of view, it should appear to do just one thing (or, perhaps, more accurately, one set of related things). Everything tangential to that responsibility should be in another class.

My only real objection with your code is that the caller needs to provide a Logger instance. It'd be better if your QueryHandler used a global Logger instance, or perhaps used inheritance or a mixin or something. I feel like the caller shouldn't need to provide a Logger, because now you've got to pass the Logger from layer to layer, making logging everyone's responsibility. However, directly speaking to "should QueryHandler log errors," it definitely falls within the scope of QueryHandler's role in SRP, much like credit card processing is in a restaurant's responsibility of serving customers food.

Answer 9

I agree with the answers already that you should take these principles pragmatically so I personally think your code is fine however I'd like to take a stab at where I think violates SRP just to play devil's advocate.

  if sc != 200:
    self.logger.warning(f'{self.query.id} returned with code: {sc}')

I would argue this branch of your query handler will change for logging reasons:

• Changing the language used to describe a failed request
• Changing what information about the request is exposed

The QueryHandler class itself seems like it should be focussed solely on executing queries. If you introduced query handling logic elsewhere or query handler subclasses I could also see the else block being duplicated.

One solution might be to move more responsibility into the logger so you end up with something like:

if sc != 200:
  self.logger.log_query_error(query, r)

or even:

  self.logger.log_query_response(query, r)

With a more declarative logging structure you now have more freedom to change your logging behaviour and your query handler can stay the same. All decisions based on what should be logged or the language that should be used is encapsulated inside the logging object.

Answer 10

The top answers here are good, but I wanted to add another way of addressing this with your coworker.

a colleague, specifically focused on this method, cites that, because the class calls the self.logger.warning, it violates the single responsibility principle (SRP)

"Logging" is very vague, and your coworker isn't correctly distinguishing between knowing how to log something, and deciding to log something (and what to log).

If it were the former, he'd be correct. But this case is the latter, to which SRP does not apply. There is no meaningfully distinct responsibility to be found inbetween the QueryHandler and Logger classes.

As is always the case, contextual complexity applies. You can't just extend that conclusion across the board for every situation. For example, if the to-be-logged message needed some non-trivial composition, you'd expect that responsibility to be ferried off to a class of its own.
In that case, it'd be an interaction of three classes: QueryHandler tells QueryHandlerLogMessage to generate a relevant log message, and then QueryHandler passes the generated message on to Logger, who in turn logs the message passed into it.

So, your coworker is right that some things should be considered as responsibilities:

• The know-how on how to log data
• A potentially complicated message generation algorithm

Neither of these would have any place in QueryHandler. However, in your case, the message is trivial, and the only thing QueryHandler does is decide to log a message, and what that (trivially generated) message should be, so SRP is not being violated.