The “save” method is a dual insert/update method. It calls an insert method. If there is a fatal error, the code “assumes” you are doing an update and calls the update method as a fatal error side effect of a failed insert. I say “assumes” because it makes no determination why the insert failed.
Your comments…
One version
public function save($record) {
try {
if ($record[$this->primaryKey] == '') {
$record[$this->primaryKey] = null;
}
$this->insert($record);
}
catch (PDOException $e) {
$this->update( $record);
}
}
Another version
public function save($record) {
$entity = new $this->className(...$this->constructorArgs);
try {
if ($record[$this->primaryKey] == '') {
$record[$this->primaryKey] = null;
}
$insertId = $this->insert($record);
$entity->{$this->primaryKey} = $insertId;
}
catch (\PDOException $e) {
$this->update($record);
}
foreach ($record as $key => $value) {
if (!empty($value)) {
$entity->$key = $value;
}
}
return $entity;
}
What exactly do you want feedback on? Normally someone asking for feedback posts some code they wrote or changes they have made to some existing code. What is your question about this class?
Well, the assumption that a failed insert means that you must be wanting to do an update and about using a fatal error catch block to do updates. Unless, there is something I don’t know, that makes no sense and is a misuse of Exception Error Handling.
This allows a form submission to execute the code:
$joke->save($_POST['joke']);
If $_POST['joke'] is:
['id' => '',
'text' = 'Why was the empty array stuck outside? It didn\'t have any keys'
]
it will run the query:
INSERT INTO `joke` (id, text) VALUES (:id, :text)
The insert will be successful and the joke added. If, however, there is an ID set:
['id' => '8',
'text' = 'Why was the empty array stuck outside? It didn\'t have any keys'
]
It will try to do an INSERT but this may or may not be successful: It may cause an error duplicate primary key. At which point it updates the existing record.
UPDATE `joke` SET text = :text WHERE id = :id
If the update then fails, another exception will occur and will not be caught.
tl;dr it means you can have a single page which works for both add and edit rather than requiring an addjoke.php and editjoke.php with very similar code. This is covered in detail in the book
I am all about the single page for adding and editing and do it myself but a much different implementation.
The part I would like to know what you have to say about is the assumption that the exception was caused by a duplicate key. A query can fail for any number of reasons.
To just automatically then attempt an insert doesn’t make sense. If the error was checked for 23000 first then that would be something else but it doesn’t. If I am reading correctly, neither the update method nor the query method that it calls has any built in error handling.
So, if the query fails for some reason other than a duplicate key, an update is then attempted which at this point is also very likely to fail.
Yes, you’re correct. And it will throw its own exception.
This is marginally inefficient: if the insert fails for any other reason it will send two queries to the database and both will fail. However, the cases where this is an issue are likely to only happen during development and the only downside is only a very minor performance overhead of an additional failing query.
If the query fails for another reason e.g. Column X does not exist in table this error is very quickly detected anyway and any potential performance loss is incredibly minor.
On the other hand, checking for error 23000 assumes you’re using MySQL. If you changed the database connection to use SQLite or Oracle instead, then the code stops working. I prefer erring on the side of flexibility here.
Yes, that error does depend on Mysql, but in the true fashion of OOP you would code against an Interface and have a different CRUD class for Mysql and say MongoDB, and another for SQlite. That is where you get your flexibility.
Which adds a huge amount of additional code and complexity for code aimed at beginners for very little practical benefit
edit: There is no real benefit in checking the error code here anyway. As you say, if the insert fails for any other reason, so will the update. So what? As I said, it’s a minor performance issue that will almost always only come up during development. This performance loss could happen 10,000 times and still not add up to the time it takes you to write the code checking the error code.
I am assuming when you say “little practical benefit” your are referring specifically to beginners.
Don’t get me wrong, I am not knocking the book. I like what was done with the Dynamic query’s and would even use it myself. I also don’t expect any book to cover everything for everyone. My discussion here is related directly to the code in this class, specifically one method, not how or why it was used in the book.
I am not going to get into performance issues and never mentioned anything about that. IMHO, if you your doing an update, then call an update method, if your doing an insert, then call your insert method. They are two completely different DB operations. You already have those two methods, so why make yet a third one? That just makes much more sense to me personally and is in line with KISS.
Then what exactly is your question here? Can you be more specific about what “feedback” it is you want on this function?
The problem with this approach (again, addressed in the book in detail) is that the calling code must know whether it wants to do an insert or an update.
That is, every single form you create needs to repeat the logic.
Add/edit joke form submission code:
if (count($_POST) > 0) {
if (empty($_POST['joke']['id']) {
$jokeTable->insert($_POST['joke']);
}
else {
$jokeTable->update($_POST['joke']);
}
}
Add/edit category form submission code:
if (count($_POST) > 0) {
if (empty($_POST['category']['id']) {
$categoryTable->insert($_POST['category']);
}
else {
$categoryTable->update($_POST['category']);
}
}
Instead, by abstracting that into a save function, the individual form submissions look like this:
add/edit joke:
if (count($_POST) > 0) {
$jokeTable->save($_POST['joke']);
}
add/edit category:
if (count($_POST) > 0) {
$categoryTable->save($_POST['category']);
}
Which of these is inline with KISS? And more importantly, DRY?
The example you gave is not what I would do. There is no reason the logic needs to be repeated as you show.
A simple refactoring of the save method and adding one parameter to the call will keep you kiss’ing, DRY, and avoids doing updates based on a fatal error.
You will also want to refactor the insert method with an ON DUPLICATE KEY UPDATE
The following logic is exactly the same as is currently in the class, we are either adding or updating.
public function save($record, $action) {
$action = 'add' ? $this->insert($record) : $this->update( $record);
}
The following logic is exactly the same as is currently in the class, we are either adding or updating.
No, it’s not. You still need to specify whether you are adding or updating. Whatever is calling ->save must specify whether it’s adding or updating. You still need the logic:
if (count($_POST) > 0) {
if (empty($_POST['category']['id']) {
$categoryTable->save($_POST['category'], 'add');
}
else {
$categoryTable->save($_POST['category'], 'update');
}
}
If you do that, you only need the insert function and do not need to specify whether you are updating or inserting so the save function you wrote above is redundant, everththing would be handled by insert. However, it’s MySQL only. The approach in the book will work for almost every DB driver PDO supports.
Do you have a question or is this just a “That’s not the way I’d have done it” post? There’s an old saying that I’ll butcher: Give 20 programmers the same problem and you’ll get 21 different solutions.
I like using ON DUPLICATE KEY UPDATE when I know the code will be run only with MySQL. But because it is a MySQL extension of the standard SQL INSERT I don’t use it if I think there’s any chance a different database will be used. (Similarly I don’t use Postgres specific functions that I like unless I know the code will only ever be used with Postgres.)
I guess that could have been coded in and it could have been made clear that because non-standard SQL is used using MySQL is a requirement.
But as Tom posted, the gains wouldn’t be all that significant and it would arguably be better to not teach non-standard SQL
@Mittineague,
Your comments go back to what I brought up earlier in the thread about coding against an Interface and having CRUD classes for the DB’s you are using. It is basic OOP.
So Mysql only supports certain things. Doesnt matter, you called the Mysql Class if that’s what you are using. So Postgres has it’s own special things, doesn’t matter, you call the Postgres Class if that’s the DB you are using. Not using SQLite right now, no need for the class, but when you do want to use it all you have to do is code against the DB Interface and use it. Nothing breaks and you dont need to touch any other code. All PDO cares about is if you provide it a valid DB Object. It doesnt care what DB it comes from, and it shouldnt.
But this adds significant complexity for a learning tool aimed at junior developers. And, as you’re fond of KISS why would you write specific code for each database implementation when you can write code that is database agnostic instead? Seems like it would be more code to try to make use of non-standard features.
If you want to take a look at a production capable ORM I wrote that follows the same philosophy take a look at https://github.com/Level-2/Maphper which does, as you suggest, use interfaces for abstracting different data sources.
To be honest I am still a little confused about what it is you are asking. Do you have a specific question about the code?
I dont know where you are coming up with junior developers. I am strictly talking code.
I thought I was clear about the class code. I wanted to know why the method assumes that a failed query means the failure came from a duplicate key error when the code doesn’t know and why an update is triggered by a fatal error in a catch block.
The reason I asked for “feedback” is because I didn’t want to jump to any conclusions in case there was something I didn’t know. I know a lot, but I don’t know everything.
I now have a new question. Can you cite any “reputable” code source anywhere that uses fatal errors for normal DB updates? I have never seen it and it goes against everything I know about errors, error reporting, and exceptions.
Reading between the lines, you seem to be implying that there is a problem with the try/catch approach used, if you are making that case burden of proof would suggest that you to demonstrate (with comparable code samples) any issues this approach introduces and how they can be avoided with an alternative method.
that uses fatal errors
An exception is not a fatal error, it’s a message back from the code that throws it telling the calling code it was unable to do what the caller expected it to do.
Why yes I do but not for why you might be thinking. It is for the redundant false.
The if function by itself ALREADY checks for true/false, well, truthiness anyway. If something is not true then it must be false. While minor, that code should be
if (!$this->insert($record))) {
$this->update($record);
}
Translated: if the statement is not true, do something.
What you posted translates: if it is true that the statement is false, do something