Single Responsibility Principle Implementation

PHP
1

I have a SQLStatement class which builds up sql query and supports bind parameters (which is stored to the property $params).
I have a PDOAdapter class which acts just like PDO but has additional features such as prepare_query(SQLStatement $sql). It prepares the sql statement and binds the parameter which is stored in $sql->params, then execute it.

And,

I think I want to add a QueryEngine class which can select, insert, update, delete which means it’ll build up SQL statement using the SQLStatement class and make PDOAdapter prepare_query it.

I can say its responsibilites are to generate sqlstatement based on the method parameters using SQLStatement class AND make PDOAdapter process it. (Multiple Responsibility)
But, I can also say, its responsibility is to do basic CRUD operations. (Single Responsibility)

Does this QueryEngine class violate Single Responsibility Principle ?
Can we say that this QueryEngine class is a Controller in MVC pattern?
Actually, what is the exact definition of the word Single?
I’m confused whether moving a table is single responsibility or multiple responsibility (lift the table up, move it, and put it down).

2

It’s difficult to answer directly without seeing the code but generally a responsibility is a reason to change (See http://www.butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod )

So consider a class which does JSON encoding. At first glance, decoding looks like a separate responsibility. However, if the encoding format changes, then the decoding function needs to change as well.

In your case: The CRUD operations are a single responsibility. UPDATE/SELECT/DELETE/INSERT methods will all need to change if you, for example, use a file to store the data rather than a database.

However, (and this is a judgement call without a better understanding of the methods in each class) it sounds like your SQLStatement class is a different responsibility… in fact I’d argue it’s more likely the same responsibility as the PDOAdapter class.

3

My SQLStatement has select(), insert(), update(), delete(), where(), innerjoin(), having(), orderBy(), groupBy(), and other methods just to build up the query and store it in $sql property, and also store the user input value in $parameter property. It’s like the Doctrine QueryBuilder

My PDOAdapter has connect(), query(), prepare_query(), exec() and disconnect() methods

Is my design bad ?

4

Having seen the methods that sounds like a sensible approach to me! A good question to ask is: “Can I substitute components?” So “Can I use a different query builder with the database adapter?” in this case the answer is “Yes” which means the design is flexible.

2 Likes
5

A trick I’ve been using lately to decide if a class is taking on too much responsibility is to ask if every method makes logical sense to invoke at any time during the object’s life. To illustrate, let’s look at the built-in PDO Statement class under this lens. It has methods such as bindParam and execute, but it also has methods such as fetchAll and rowCount. The latter two don’t always make sense to invoke at any time on a statement.

// Execute then fetch; makes sense
$sth = $dbh->prepare("SELECT name, colour FROM fruit");
$sth->execute();
$result = $sth->fetchAll();

// Fetch then execute? That doesn't make sense; I smell a separate responsibility
$sth = $dbh->prepare("SELECT name, colour FROM fruit");
$result = $sth->fetchAll();
$sth->execute();

The built-in PDO Statement class might benefit from the addition of a ResultSet class. The methods fetchAll and rowCount would be moved from Statement to ResultSet.

// Hypothetical example
$sth = $dbh->prepare("SELECT name, colour FROM fruit");
$resultSet = $sth->execute();
$result = $resultSet->fetchAll();

From the moment a ResultSet is constructed, the methods fetchAll and rowCount will always make sense. And Statement is now rid of the methods that don’t always make sense.

First and foremost, come up with a better name. “Engine” is much too vague. What concept will this class represent? Or what task will it do? So far, it sounds like you’re describing a “repository” concept.

A repository would still be part of the model.

“only one” :wink:

1 Like
6

Interesting point and I agree with your solution but this seems more like an encapsulation issue than an SRP one. I’ve never really thought about encapsulation in terms of SRP before but you make a great point about their relationship.

Given your example:

// Execute then fetch; makes sense
$sth = $dbh->prepare("SELECT name, colour FROM fruit");
$sth->execute();
$result = $sth->fetchAll();

The person writing this code needs to mentally keep track of the state of $sth (has it been executed yet or not?). This exposed state is broken encapsulation.

This is also why setter injection is bad:

$car->setEngine(new Engine);
$car->drive();

If the drive method needs the engine instance, then it won’t work unless the methods are called in the correct order. In this case, it’s not a SRP problem, it’s 100% encapsulation. The answer of course is constructor injection because it makes it impossible for a $car instance to exist where calling the drive method would fail.

I think it’s “Responsibility” which is more difficult to concisely define. Your point about “if every method makes logical sense to invoke at any time during the object’s life” is, imho, better than the standard “requirement to change” definition. If A needs to happen before B then A and B are different responsibilities.

1 Like
7

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.