Really? We’re being that petty are we? Let’s keep it conceptual rather than irrelevant semantic differences.
If you don’t have an issue with the logic in that block, then you have a misunderstanding of how exceptions work or what they are intended for. The logic is the same whether you are using error codes (returning null/false):
try to insert
if the insert fails, update instead
Whether this is done with try/catch or if/else is an implementation detail which in this case is dictated by the fact that $pdo->query may throw an exception.
I will take a look at the link after I get some sleep and a fresh pair of eyes. I don’t know that we are going to come to a meeting of the minds about the current save method implementation but I am open to hear what you have to say about it.
Right now, the method indisputably REQUIRES there to be an error in order to work properly. It doesn’t matter if you call it a fatal error or not, it is still an error and it is required for normal program flow. It just makes no logical sense whatsoever to code an application that requires errors to work correctly. That is a code smell no matter how you slice it.
I think this is where you misunderstand what is happening. It does not require an error to work properly. If the insert is successful, there is no error. If the insert cannot be performed, an update is done instead. Exceptions are used to detect whether the insert was successful or not, nothing else. This is logically no different than PDO returning false rather than using an exception, which you don’t seem to have an issue with.
In some manner PDO needs to inform the calling code whether the query was successful or not. It doesn’t matter whether this is done via return statements or exceptions the result is the same: the calling code is informed that the query was unsuccessful .
Please take a look in more depth about how exceptions work and what they are for. Exceptions are not errors, uncaught exceptions are errors.
I do, it violates Command Query Seperation as your command method is returning a result. It should either return nothing in case of success, or throw an exception in case of a failure.
Why not check if an id was posted, and if it was let save call update, or otherwise insert?
function function save($data) {
if (isset($data['id'])) {
$this->update($data);
return;
}
$this->insert($data);
}
(that does require all tables to use id as primary key, but that can be made configurable as well when you go this route)
You are correct of course (and basically what the links I provided said), but one step at a time, this is the point I’m trying to make here.
Yes, that’s one way to do it and I had considered it but:
It assumes all tables use auto-increment, if you have a table where the PK is a a user-supplied value to be used when inserting the record, the method breaks
They are pretty self evident. If you have a table where StockCode is the primary key and comes from a value entered by the user into the form, it will never be able to insert because if (isset($data['StockCode'])) will always have a value and update will always be triggered.
If your table has a composite key say jokeid, categoryid this breaks:
Depending on which column is being checked in the save method, this will likely do one insert and one update rather than two inserts.
You could add some logic to handle this but it starts getting needlessly complex (and still won’t fix the problem of tables that aren’t auto-increment)
I understand perfectly what is happening. You are using exceptions for normal program flow. Doing an insert or update is not an exceptional task. Would you really argue that it is?
That is just not true. To do an edit, the code REQUIRES a duplicate key insert attempt which causes ERROR 1062 which in turn causes the Exception in Php which then calls the update method in the catch block. A succesful edit is more than tightly coupled to a database error, it is ABSOLUTELY dependent on on it. The code proves it. It just makes no logical sense whatsoever for an application to require a DB error in order to work properly.
And herein lies the problem and the bug. The code assumes the failure is from a duplicate key error when in fact, it has no idea why it failed. We do not code decisions on assumption. There is no maybe_maybe_not function. We make program flow decisions based on a concrete code status.
Lets take the case of doing an insert. When the insert query fails for some reason other than a duplicate key error the code now silently attempts an update which also fails. What are you updating? I am doing an insert, there is nothing to update. What in the world are we doing in an update method? Again, it makes no sense at all and smells really bad.
And all this for what? For what you believe DRY means? DRY is beyond duplicate code. I will refer you to the Wiki.
“In software engineering, don’t repeat yourself (DRY) is a principle of software development aimed at reducing repetition of software patterns”
“The DRY principle is stated as “Every piece of knowledge must have a single, unambiguous, authoritative representation within a system”.”
The first thing we need to do is identify the problem to be solved. Best I can tell from your posts, it is avoiding code duplication to adhere to the DRY Principle. Correct me if I am wrong, but it seems that you are tightly interpreting DRY to mean code duplication specifically and not what the “official” description of it is from the people that actually formulated it.
If we go with the official description is there really a problem that needs to be solved with some special handling? What we are dealing with here really couldn’t be any simpler. Either we want to do an insert or we want to do an update. You already have two well written methods to do that. Just use them.
I have laid out my case against the current implementation. Really nothing more I can say.
PDO sends back the fact the insert failed using an exception. I need to detect that. Once again, it makes no difference to the logic of the program whether PDO gives this information as an exception or a false return value. My code accesses this information in the format it is supplied (exceptions) and acts accordingly.
An exception is not an error.
“logical sense”. The logic here is “Is there an existing record in the database with this primary key”? How that is handled is an implementation detail. I could use a SELECT query (another trip to the DB and more code), PDO could return false or throw an exception. It doesn’t matter, it’s an implementation detail.
Please show an example of how this is a bug: Please provide a test case where it produces unexpected behavior (with pdo set to throw exceptions)
Wrong. It does not matter why the insert failed. In fact, being agnostic to it is a benefit here as we let the database handle database operations rather than trying to second guess it. Different database drivers will throw different errors, by being agnostic to why it’s considerably more flexible.
And what happens then? The exception is thrown and unless caught higher up, shown to the developer or logged. The exact same thing that would have happened if you were just doing an insert but not catching the exception. The programmer still knows if the record was saved successfully because the save method still throws an exception if both update and insert failed.
And it’s only during development that two exceptions are even likely to occur. On the production site (malicious users aside), either one of the queries will execute as intended.
There is literally no downside to this approach. The only viable alternative is:
SELECT count(id) FROM table WHERE id = :id
If there is a record perform an insert, otherwise perform an update.
Inserts require two trips to the database
Updates require two trips to the database
It requires extra logic to determine if the record exists
It requires additional logic to generate a SELECT query that supports composite keys
The update or insert might still fail: what in the world are we doing in an update method?
On the other hand:
Try to insert
Otherwise update
Inserts require one trip to the database
Updates require two trips to the database
Works with composite keys
Doesn’t require the logic for determining whether the key is in use
No, you have said you don’t like it. You have not provided any concrete examples of why it’s an issue.
Once again I will point out that
if (!$this->insert($record)) {
$this->update($record);
}
is not any logically different to try/catch. In this example the update occurs if the insert fails for any reason. The only difference is how PDO reports the fact that there was an issue with the insert.
Please read the earlier posts. We cannot “just use them” without repeating the “If there is a record in the table with this ID update, otherwise insert” logic.
As I said before: burden of proof would suggest that you demonstrate (with comparable code samples) any issues this approach introduces and how they can be avoided with an alternative method.
Unless you can provide an alternative approach (which doesn’t rely on duplicating the “does the record exist” logic) and explain why it’s better all I can see here is you are irrationally getting upset because you don’t like the way I wrote the code.
Nothing personal Tom, but to be fair I would almost certainly have written bits and pieces differently for no reason other than personal preference and habit. But then, this is a truth
I was talking about editing, not inserting. If you are doing an edit there is no reason to care anything whatsoever about an insert.
Please re-read what I posted. The error is a database error and it even has it’s own ERROR code number, 1062. And a successful edit does in fact absolutely depend on it in order to work. The update will not execute without it.
Again I am talking about editing so that is not the logic at all. I already know there is an existing record otherwise I would not be editing it.
Thats easy. Just rename joke.joketext to have an X at the end to simulate a query failure other than a duplicate key constraint error. With production settings turned on you would get a blank page and no indication as to why. Pretty sure that would be unexpected behavior from the users point of view.
A software bug is an error, flaw, failure or fault in a computer program or system that causes it to produce an incorrect or unexpected result, or to behave in unintended ways.
But, but, I am talking about doing an edit. Why do I have to care about inserts???
But, but I just want to do an edit. Why are we still talking about inserts??? Why do I have to care about inserts? I just want to do my edit.
Really? Because the whole implementation depends on it failing for a particular reason in order to do an update. If it fails for any other reason, the whole thing fails. That is a bug.
You are writing code smells for the sake of multi DB “flexibility” when you should be coding to an interface and having the proper DB object available for the particular DB you are using. “Flexibility” sure sounds alot like more than one responsibility.
Not sure where you get two exceptions. All it takes is one failed insert that is not from a duplicate key error and the built in bug shows its face. Are you saying you write your code based on your perceived likelihood of something happening instead of that it CAN happen?
Are you really saying that on a production server there will never be an unintended query failure? I would like to rent that server please. And since when can we ever put malicious users aside?
But, but, what about the bug that I described? It’s really there. I described a very easy way to simulate it.
As far as viable alternatives I will see about posting something. I will consider your selling points in whatever I show. It seems you are stuck in the implementation of the caller which seems to be limiting “viable alternatives”. I haven’t had time to review that code yet so I can’t comment on that.
I keep telling you, I just want to do my update. I don’t care about an insert. Why do you insist on making me care about it?
I am not even close to upset. I love talking code, especially at a higher level as we are doing. Hopefully I will learn some new things. @rpkamp has already learned me up about “Command Query Separation”. Never heard the term before this thread, but the description makes a lot of sense. Trust me, I am all about adopting better practices and refactoring my code as I learn new and better techniques. I am just not seeing it in this one methods implementation, but I am listening. If I had any issue it would just be that this coding method is being taught in a book aimed at beginners (and up).
If you would please, clarify exactly what problem you intended to solve with this particular implementation. It will help coming up with a “viable alternative”.
This, I think is the point you are missing. When the user submits the form, I do not know if they are doing an update or an insert, I don’t want to have to write code specifically for edit or for insert. The user submits the form, the record is sent off to the database.
Whatever argument you are making here also applies to ON DUPLICATE KEY UPDATE.
A bug is a situation where the code not work as intended. This is not a bug.
If you think it is, please demonstrate a situation where the expected behaviour does not occur. This is not a bug, it’s a try/catch statement you don’t like for who knows why.
This will happen if you use only the insert or update function alone (not through save)
Only if you have your website’s error handling set up poorly. A try/catch should be set up around the entire site so that errors can be displayed in a more meaningful way. On production, just a generic error page and logged, on dev shown to the developer.
If someone renames a form field in the browser they should expect things to break once it’s submitted
You haven’t made a case here at all. You dislike try/catch for who knows what reason and you’re poorly trying to argue against its use. I’ll ask again, do you have a problem with this code:
if (!$this->insert($record)) {
$this->update($record);
}
This is identical: It tries to insert, if the insert fails it does an update. In the same way it is still “relying on an error”. It doesn’t matter if the try to insert otherwise update logic is an if statement, a try/catch or ON DUPLICATE KEY UPDATE
If my code looked like that originally I guarantee you never would have questioned it.
All that is happening here is a database agnostic ON DUPLICATE KEY UPDATE.
See my first post in this thread: Make it so that the logic of if there's an existing record, update, otherwise insert does not need to be repeated. One approach we already discussed (and you don’t seem to mind) is ON DUPLICATE KEY UPDATE. I’ve told you why that’s not a viable solution. The try/catch approach is just a database agnostic version of that. Any argument you make about “not wanting to insert only update” also applies to ON DUPLICATE KEY so you’re at a dead end there because you suggested that earlier.
You entire argument seems to be “I don’t like it” because you have some weird aversion to try/catch that you haven’t been able to explain. I’ve shown you the benefits of this approach and you won’t provide an example of where this approach causes any kind of practical issue (beyond what the insert function would on its own if given invalid data). I’m done with this thread until you can do that.
Hi @TomB,
Thanks for the detailed responses. I could show you a simple single add/edit implementation but I think based on your replies you may consider it violating DRY if your definition is not what the founders of it say it is. My problem is not necessarily the try/catch. It is that this implementation requires doing an insert attempt in order to do an edit and that the code assumes the failed insert is from a duplicate key constraint error and doesn’t handle the case when it is some other reason for a failed insert.
Let me ask you this, roles and permissions are a very real part of applications. What happens when my role or permissions only allows me to edit? Now I am screwed because in order for me to do my edit only job I have to also have insert permissions. What now Tom? How would you deal with that with the current implementation?
Anyway, after reviewing a bit more of the code it seems you have something going on close to what I would propose as the solution. In it’s simplest form, if there is a GET id you know you are getting data to doing an edit. The GET id would populate a hidden id form input. When you have a POST with the hidden id value then the code knows to do an update. If there is a POST with no value for the hidden id you know it is an insert. Very simple really. Just a little if/else. It should make sense but I can do a code example if need be.
By the way, the implementation I will post tomorrow does not require any changes to the current Class, it just won’t use the save method.
Aw man, I remember you challenging @John_Betong to put some code up on GitHub (here) and he did. I’ve been following this thread and it would be slightly disappointing if you don’t put your money where your mouth is.
OFF TOPIC: Sure, I’ll post it tomorrow. It’s a bit late in the evening right now. You know, I didn’t see that he posted that. He waited some time to do it I will have to check it out. Hardly a challenge though, but I think I get what your getting at. I don’t want to disappoint you.
This is a red herring. Permissions checks happen before any data is sent to the database for storage so it’s the same whether you are using save or update.
I will remind you once again that it’s a book aimed at teaching novices the concepts and I purposely didn’t want them to get bogged down with unnecessary complexity.
I completely understand Tom. As I posted earlier, my posts are about code, rather one method in particular, not the book itself. After reading several chapters I am quite impressed with the level of expertise it shows and have already learned a couple new techniques. Well done!
Really? We’re being that petty are we? Let’s keep it conceptual rather than irrelevant semantic differences.