Re: [Koha-devel] Question on proper/preferred DBI usage?
I definitely do not want to get into a flame war on this, so if it turns that direction, I'll drop the subject. On Thu, May 16, 2002 at 03:42:30PM +1200, Gynn Lomax wrote:
The reason it has been done this way is simple: I like to be able to see what values I am assign to where.
With the values embedded into the query it means I can directly see what they match up with, e.g.:
$query = "Insert into biblioitem set biblionumber = $biblioitem->{'biblionumber'}, itemtype = $biblioitem->{'itemtype'}";
I haven't looked at enough of the code, but I haven't see much of that in the code. I see things like: $sth=$dbh->prepare("insert into biblioitems (biblioitemnumber, biblionum ber, volume, number, itemtype, isbn, issn, dewey, subclass, publicationyear, pub lishercode, volumedate, volumeddesc, illus, pages, notes, size, place, lccn, mar c) values ($biblioitemnumber, $biblionumber, $q_volume, $q_number, $q_itemtype, $q_isbn, $q_issn, $q_dewey, $q_subclass, $q_publicationyear, $q_publishercode, $ q_volumedate, $q_volumeddesc, $q_illus, $q_pages, $q_notes, $q_size, $q_place, $ q_lccn, $q_marc)"); and I'm definitely having a hard time matching up which variable goes with which column name.
With the bind values method you get: $query = "Insert into biblioitem set biblionumber = ?, itemtype = ?";
followed by later: $sth->execute($biblioitem->{'biblionumber'}, $biblioitem->{'itemtype'});
While for this simple example it is easy to see which value matches up, for more complicated queries, and those that have a lot of values it can start to get mor confusing.
With this method, as well as the example above (straight from marcimport.pl), in my opinion the main problem is formatting. Compare, for example, the following actual code: ------------------------------------------------------------- my $q_isbn=$dbh->quote($isbn); my $q_issn=$dbh->quote($issn); my $q_lccn=$dbh->quote($lccn); my $q_volume=$dbh->quote($input->param('volume')); my $q_number=$dbh->quote($input->param('number')); my $q_itemtype=$dbh->quote($input->param('itemtype')); my $q_dewey=$dbh->quote($input->param('dewey')); my $q_subclass=$dbh->quote($input->param('subclass')); my $q_publicationyear=$dbh->quote($input->param('publicationyear')); my $q_publishercode=$dbh->quote($input->param('publishercode')); my $q_volumedate=$dbh->quote($input->param('volumedate')); my $q_volumeddesc=$dbh->quote($input->param('volumeddesc')); my $q_illus=$dbh->quote($input->param('illustrator')); my $q_pages=$dbh->quote($input->param('pages')); my $q_notes=$dbh->quote($input->param('note')); my $q_size=$dbh->quote($input->param('size')); my $q_place=$dbh->quote($input->param('place')); my $q_marc=$dbh->quote($input->param('marc')); $sth=$dbh->prepare("insert into biblioitems (biblioitemnumber, biblionum ber, volume, number, itemtype, isbn, issn, dewey, subclass, publicationyear, pub lishercode, volumedate, volumeddesc, illus, pages, notes, size, place, lccn, mar c) values ($biblioitemnumber, $biblionumber, $q_volume, $q_number, $q_itemtype, $q_isbn, $q_issn, $q_dewey, $q_subclass, $q_publicationyear, $q_publishercode, $ q_volumedate, $q_volumeddesc, $q_illus, $q_pages, $q_notes, $q_size, $q_place, $ q_lccn, $q_marc)"); ------------------------------------------------------------- with this functional equivalent: ------------------------------------------------------------- $sth=$dbh->prepare("insert into biblioitems (biblioitemnumber, biblionumber, volume, number, itemtype, isbn, issn, dewey, subclass, publicationyear, publishercode, volumedate, volumeddesc, illus, pages, notes, size, place, lccn, marc) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) "); $sth=$dbh->execute( $biblioitemnumber, $biblionumber, $input->param{'volume'}, $input->param{'number'}, $input->param{'itemtype'},$input->param{'isbn'},$input->param{'issn'}, $input->param{'dewey'}, $input->param{'subclass'}, $input->param{'publicationyear'}, $input->param{'publishercode'}, $input->param{'volumedate'}, $input->param{'volumeddesc'}, $input->param{'illustrator'}, $input->param{'pages'}, $input->param{'note'}, $input->param{'size'}, $input->param{'place'}, $input->param{'lccn'}, $input->param{'marc) ); ------------------------------------------------------------- I'm sure it is just a matter of what each person is accustomed to, but I can match up parameters easier in the second example than I can in the original.
For the very few queries I have run across that are executed more than once, the values being put into them are an unknown list (in terms of length), e.g.
$query = "Insert into deleteditems values (" foreach my $value (@results) { $value = $dbh->quote($value); $query .= "$value,"; } # foreach
and I'm unsure if the bind values method can handle this, so avoided it.
This code snippet looks like it is inserting an "unknown list" of column values into a table, on the assumption that the unknown list just happens to match up with the exact correct order of columns in the table. This is different than the original issue which is in the context of inserting multiple rows, not columns. I'm definitely going to be stepping into hot water here, and so I must apologize in advance for that. But as to the columns, it seems to me to be unsafe coding practice to insert values into a table without column names, because somebody somewhere is going to do some db modification, whether through custom changes or version upgrades, and some column will missing or out of order and it will break. Hashes perhaps are a good solution for this type of thing. In any case, the bound parameter method works just fine for this: $query = "Insert into deleteditems values (" foreach my $dummy (@results) { $query .= "?,"; } # foreach $query=~s/,$/\)/; $sth->execute(@results); And there is probably a shorter way to do the correct number of question marks via a count of @results.
The other advantage of the method I have used is that you can print out the entire query before it is executed - so you can examine it for bugs. Whereas with the bind value method you never get to see the query in it's entirety.
OK, with the quoted method you only have to print one thing, while with the bound method you have to print more. That's true. I sure don't want to start a flame war, and if it sounds like I am, please just ignore me and I'll drop it. Thanks for the education on another point of view. - Alan -- ---- Alan Millar --==> am12@bolis.com <==--
Alan Millar wrote: <SNIP>
I'm definitely going to be stepping into hot water here, and so I must apologize in advance for that. But as to the columns, it seems to me to be unsafe coding practice to insert values into a table without column names, because somebody somewhere is going to do some db modification, whether through custom changes or version upgrades, and some column will missing or out of order and it will break. Hashes perhaps are a good solution for this type of thing.
In any case, the bound parameter method works just fine for this:
$query = "Insert into deleteditems values (" foreach my $dummy (@results) { $query .= "?,"; } # foreach $query=~s/,$/\)/; $sth->execute(@results);
And there is probably a shorter way to do the correct number of question marks via a count of @results.
I STRONGLY AGREE WITH YOU. ============================ I've submitted a patch a few days ago, modifying the "deletedbiblio" table. This patch was needed because of such an "insert into XX values (XX)" that was not working properly... Note also it's not recommended to do a 'select * from ...". We should always do "select field,field,field from ...". 2 reasons for this : * the DBI structure permits use of another DB (in theory at least). Some databases don't enjoy a "select * from table1,table2 where join_between_table1_and_table2". the need "select table1.*,table2.* from ..." * If we build joins of 3 or more tables, the resulting row can be quite huge... A third argument : * if we have fields with the same name in a multi-table select, a select * results in a "ambiguous clause" failure, or something wrong like this (note it can be avoided if you plan to have all your fields differents)
I sure don't want to start a flame war, and if it sounds like I am, please just ignore me and I'll drop it. Thanks for the education on another point of view.
It's not a flame war beginning, as you don't say we are "dummies", "stupids", "we MUST do it as you want"... ;-) We are speaking of good coding practices. -- Paul
On Wed, 15 May 2002, Alan Millar wrote:
I definitely do not want to get into a flame war on this, so if it turns that direction, I'll drop the subject.
No flame war from me. I didn't know about the '?' method. I like it a lot, but did you _have_ to use _my_ code as an example? ;) As far as indenting goes, Paul, I'm with Chris, except I do whatever vim does by default. I'm all for setting a standard, I'm sure I can make vim do whatever we decide. Steve.
my $q_isbn=$dbh->quote($isbn); my $q_issn=$dbh->quote($issn); my $q_lccn=$dbh->quote($lccn); my $q_volume=$dbh->quote($input->param('volume')); my $q_number=$dbh->quote($input->param('number')); my $q_itemtype=$dbh->quote($input->param('itemtype')); my $q_dewey=$dbh->quote($input->param('dewey')); my $q_subclass=$dbh->quote($input->param('subclass')); my $q_publicationyear=$dbh->quote($input->param('publicationyear')); my $q_publishercode=$dbh->quote($input->param('publishercode')); my $q_volumedate=$dbh->quote($input->param('volumedate')); my $q_volumeddesc=$dbh->quote($input->param('volumeddesc')); my $q_illus=$dbh->quote($input->param('illustrator')); my $q_pages=$dbh->quote($input->param('pages')); my $q_notes=$dbh->quote($input->param('note')); my $q_size=$dbh->quote($input->param('size')); my $q_place=$dbh->quote($input->param('place')); my $q_marc=$dbh->quote($input->param('marc'));
$sth=$dbh->prepare("insert into biblioitems (biblioitemnumber, biblionum ber, volume, number, itemtype, isbn, issn, dewey, subclass, publicationyear, pub lishercode, volumedate, volumeddesc, illus, pages, notes, size, place, lccn, mar c) values ($biblioitemnumber, $biblionumber, $q_volume, $q_number, $q_itemtype, $q_isbn, $q_issn, $q_dewey, $q_subclass, $q_publicationyear, $q_publishercode, $ q_volumedate, $q_volumeddesc, $q_illus, $q_pages, $q_notes, $q_size, $q_place, $ q_lccn, $q_marc)");
I definitely do not want to get into a flame war on this, so if it turns that direction, I'll drop the subject.
No, it's good to discuss differences in code generation, you learn things that you might have otherwise missed :) It mightn't be such a bad idea to decide on a set way that the code should look like, that way it would be easier for someone new comming in to understand it all. Gynn.
On Fri, 17 May 2002, Gynn Lomax wrote:
I definitely do not want to get into a flame war on this, so if it turns that direction, I'll drop the subject.
No, it's good to discuss differences in code generation, you learn things that you might have otherwise missed :)
It mightn't be such a bad idea to decide on a set way that the code should look like, that way it would be easier for someone new comming in to understand it all.
Anyone want to look at perltidy? It would certainly make for a nice pre-checkin step (maybe along with running testKoha.pl). -pate -pate
Gynn.
_______________________________________________________________
Have big pipes? SourceForge.net is looking for download mirrors. We supply the hardware. You get the recognition. Email Us: bandwidth@sourceforge.net _______________________________________________ Koha-devel mailing list Koha-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/koha-devel
participants (5)
-
Alan Millar -
Gynn Lomax -
Pat Eyler -
paul POULAIN -
Tonnesen Steve