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?
Best Answer
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 calledRandom
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
, orExpensiveMachineController
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.