note afoken I literally had to look twice at the date of this posting. I had the strong feeling that I had stumbled over one of the ancient nodes created in the early stages of perlmonks.org, perhaps as a repost of much older code from the ages of Perl 4. But that posting is dated Dec 23, 2009. So, what's wrong with that code? <ul> <li>Perl 4 is dead. Don't beat that horse, it won't run any more. Prefixing a subroutine call with an ampersand does something very different in Perl 5 (disabling subroutine prototype checks) than it did in Perl 4 (just calling the function). And most times, you don't want that to happen. Drop that ampersand.</li> <li>Of course, your code explicitly states that <c>row_sql</c>, <c>get_sql</c>, and <c>hash_sql</c> DO NOT take any parameters - they have an empty prototype. Still, your code reads those parameters that should not exist. This works only when you explicitly disable prototype checks by prefixing the function name with an ampersand. Drop that empty prototypes.</li> <li>Perl 5 has modules. Don't put code into scripts loaded at runtime by [doc://do], use [doc://perlmod|modules]. This way, the namespace of the caller is not polluted. If you need to pollute the caller's namespace, use the [doc://Exporter] or newer modules like [mod://Sub::Exporter]. Preferably, export (pollute) only on demand.</li> <li>No [doc://strict], no [doc://warnings]. Both would have complained about several errors in the code.</li> <li>No way to setup the database connection, especially database driver, username and password, without changing the code for each and every application that wants to include this feature. I.e. you have to make a copy of this code for each application. This ends in a maintainance nightmare.</li> <li>High risk for SQL injection, because placeholders are not supported.</li> <li>Despite the effords to enable caching, the cache is poisoned by not using placeholders. So, performance will be far less than optimal.</li> <li>The code is limited to only one DB connection, without any obvious reason. Had this been written as a class with attributes instead of tons of global variables, this limit would not exist.</li> <li><c>print</c> and <c>exit</c> instead of <c>die</c> prevents any attempts of error recovery. And error messages end where they don't belong: <c>STDOUT</c>.</li> <li><c>eval</c> blocks that hide errors, e.g. while fetching data rows from the database. Again, this prevents error recovery, simply because nobody can detect errors! Either don't use <c>eval</c> or re-throw the errors.</li> <li>No concept of how to indent code. Instead, a wild mix of tabs, spaces, and missing newlines. This makes the code hard to read, hard to understand, hard to maintain.</li> <li>A constantly increased index counter (<c>$c</c>) in <c>get_sql()</c>. Why don't you just use <c>push @results,$row[0]</c>? <li>Reinventing the wheel in inefficient perl code. Had you read the [mod://DBI] manual, you would have found that your <c>get_sql</c> is a poor reimplementation of DBI's <c>selectcol_arrayref</c>. Of course, your <c>hash_sql</c> function has a superiour DBI equivalent: <c>selectall_hashref</c>. And your <c>row_sql</c> is the dumb cousin of DBI's <c>selectrow_array</c>. All of those DBI methods are written in fast C/XS code (with pure-perl fallback routines).</li> <li>Your code is not safe for transactions. Reconnecting to the database ends an old transaction (does it rollback or commit?) and creates a new one, damaging data.</li> <li>What is the reason for <c>del_sql</c>, <c>mod_sql</c>, and <c>put_sql</c>? They are all just wrappers for <c>sql</c>. And instead of aborting with a useful error message when called with an empty SQL statement, they print a warning and CONTINUE calling <c>sql</c>. <c>sql</c> still aborts, but gives a wrong error message.</li> <li>What's the purpose of the various <c>sleep(1)</c> calls?</li> <li>What's the purpose of <c>$retry_count</c>? It is incremented quite often, but never compared to anything.</li> <li>Why do you emit "warnings" to STDOUT using print instead of using [doc://warn]? <c>warn</c> allows your caller to handle warnings (<c>$SIG{__WARN__}</c>), and unless your caller traps them, they end where they belong: STDERR.</li> <li>[mod://DBIx::AutoReconnect] implements the only "new" feature in your code, i.e. automatic reconnect to the database. But <c>DBIx::AutoReconnect</c> implements this feature in a way that is nearly completely transparent to the application. You just add <c>use DBIx::AutoReconnect</c> to your application, and change <c>DBI->connect(...)</c> to <c>DBIx::AutoReconnect->connect(...)</c>, and every other feature of DBI behaves as usual. Of course, it works with every database, and with all <c>connect</c> parameters. It adds three additional connect attributes that allow fine-tuning. So, what is the advantage of your code over the nearly five year old <c>DBIx::AutoReconnect</c> that's available for free at CPAN?</li> <li>And finally: Use sql.pl as you wish, its [sic] now yours. This reads like: Take this crap and don't ever ask me for help. (OK, one could also read this as "I hereby put this code into public domain.")</li> </ul> I would recommend to use this sql.pl only as a example of how not to write Perl code. In clear words: DO NOT USE THIS CRAP! Code like this is responsible for Perls bad reputation. So: <tt>--</tt> Alexander <div class="pmsig"><div class="pmsig-747201"> -- Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-) </div></div> 814072 814072