31

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?

16
  • 37
    by calling the self.logger.warning 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 This is exactly right. Still, I am curious how your colleague would refactor the class to remove what he sees as the violation. In his mind, is there any way for a class to use other classes and not violate the SRP? Honestly seems absurd to me, but I'm willing to have my eyes opened. – John Wu May 27 at 16:22
  • 3
    SRP describes a responsibility as a reason to change -- would you ever have any reason to change the way your log messages are written without something else in that class also having changed? That seems fairly unlikely to me since logging is generally only for the benefit of developers/support rather than affecting functionality/behaviour. – Ben Cottrell May 27 at 16:29
  • 4
    @BenCottrell: in my experience, the "reason to change" explanation is worse than no explanation at all. – whatsisname May 27 at 16:41
  • 2
    Out of curiosity, did your colleague offer an alternative? I wonder how they would log things without ever calling a log function. – Deacon May 27 at 18:08
  • 6
    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. – Tommy Andersen May 28 at 5:15

10 Answers 10

42

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.

6
  • 1
    It's all about human perception. One cannot break everything down into atomic parts. Even the File class can be seen to be responsible to read files, write files and move files. Different responsibilities? Yes and no. I think the class itself should still be usable without a logger and return the error codes (or whatever) to it's caller as well. By exceptions or return values. – Chrᴉz says reinstate Monica May 28 at 8:08
  • 7
    Your colleague is taking the word "Single" too literally and dogmatically. Across the board, one of the problems in teaching people good coding skills and practices is that the terms and descriptions used often don't completely match what people are really trying to say, or if they do, they're still ambiguous / have multiple meanings much the time. This creates a lot of confusion and such earlier on in people's careers. – Panzercrisis May 28 at 14:13
  • "Single conceptual purpose" to me makes more sense than "one reason to change". I get funny looks from people when I say I only like bits and pieces of SOLID, it generally ends up being "oh we absolutely agree with the very common sense aspect that you shouldn't make god objects" (which I agree with). But what's a god object? "I know it when I see it.", is usually the only answer. I get the feeling that sometimes people think Bob Martin is the cliff's notes for good design, or some kind of shortcut for making the code fit what you're trying to do, it's not. – jrh May 28 at 20:28
  • For me generally a "responsibility" is usually heuristically determined, it's not a class or function or business need or button on the UI (or any concrete thing you can think of), but a way to know you might have one is when you find a lot of functions that accept the same object as a parameter. Should maybe there be methods on the class instead of this? Does the behavior belong to the class? That's a super hard question, actually, it also has to factor in whether it makes sense as a concept (can you name this object? If you can't, maybe it shouldn't be a class). – jrh May 28 at 20:34
26

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.

1
  • 2
    You're exactly right to focus on code changes -- that's what the SRP is designed to address. A "responsibility" in the SRP is a reason for the code module to change. – JounceCracklePop May 28 at 23:52
9

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.

2
  • 4
    I remember someone posting “we applied SRP and went from 70 classes to 700… How do we handle 700 classes…” – gnasher729 May 28 at 5:18
  • 2
    @gnasher729 so someone really tried ! Thanks: you made my day :-) – Christophe May 28 at 5:40
1

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.

10
  • 2
    Hm, can someone get mi enlightened by sharing what might be the cause of down vote? Except of poor formatting, since it can be fixed easily. I have no hard feelings, but downvote does not encourage me to answer further in this site. – somebody May 30 at 8:32
  • Up-vote from me. Logging is a classic cross-cutting concern, which is what aspects are designed to address. The OP is correct that there really are two things the class is concerned with, and one of them (logging) is not its main job. However, without aspects, there isn't much you can do about it, which leads to a lot of rationalization that it's not a violation of SRP. Well, it is, but it's one we have to live with, and most of the time it's ugly but not truly awful, so we get along with it alright. – Wayne Conrad May 30 at 13:45
  • I don't really see how AOP comes into play here. That is a way to do some logging, but can't really cover most logging scenarios. (Not that you are saying it is required.) The bigger issue is a misunderstanding of SRP. It's not that a method does one single thing, but that it has one reason to change, which some of the other answers hit more directly. – Xtros Jun 18 at 18:48
  • @WayneConrad "...it's not a violation of SRP. Well, it is, but it's one we have to live with" I think this re-iterates the misunderstanding of SRP. Logging is not a violation of SRP, assuming that the details of how logging works (like what target(s), if it gets logged, and in what format) are properly abstracted away. – Xtros Jun 18 at 18:51
  • @Xtros Lets consider an example where you have a logic in some method where you have to call to a slow remote service (REST, DB...). In that case you may wish to put a log saying something like this: "Loading of data has started...". This may be considered a important log in that case. Lets say that the remote service implementation has changed, so that you start using a cached values instead of slow method calls. In this case the log has little importance and you may wish to remove it. So, there you go, you have a second reason to change a method (apart from change in original method task). – somebody Jun 20 at 8:58
0

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.

0

"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.

0

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.

0

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.

0

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.

5
  • but if you move that logic to logging object, won't you have same problem that logging handler can be changed for business reasons? – Esben Skov Pedersen May 31 at 6:47
  • @EsbenSkovPedersen What part in particular would you think would change for business reasons? – CaTs May 31 at 8:14
  • 1
    I mean with your suggestion you logger would need knowledge of the fields in you object and if you were to change any of those the logger would also update. Actually the logger would need to know about every object it should log with your suggestion. Not sure that is an improvement. – Esben Skov Pedersen May 31 at 8:41
  • @EsbenSkovPedersen Needing to change how I interact with an object due to changing fields feels like the result of poor design rather than business rules to me. I would try to rethink the behaviour of my objects so collaborators are shielded from such changes. – CaTs May 31 at 13:51
  • @CatTs My point is your logger would have to know about all objects it can log which is not necessaryly better – Esben Skov Pedersen Jun 1 at 6:52
0

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.

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