Don't ask to ask, just ask | |
PerlMonks |
Re: I wrote a very ugly piece of code, can someone help me correct it?by dsheroh (Monsignor) |
on Mar 15, 2024 at 09:09 UTC ( [id://11158276]=note: print w/replies, xml ) | Need Help?? |
Two things immediately stand out to me:
1) You have "limit 1" in your first query. Since it will only ever return a single row, there's no reason to use selectall_hashref. Use selectrow_array (if you're OK with identifying fields by position) or selectrow_hashref (if you want to access fields by name) instead and then you won't need to bother with looping over the (single) returned row. If you opted for selectall/looping as a way to deal with the possibility that the query might return no rows, note that your existing code is broken for that case, since $ID will be undef when running the second query, causing it to lock all rows with NULL id fields. (You probably haven't noticed this bug because your table doesn't allow NULL ids, but it's still there.) Fetching a single row, you can check whether $ID (or another "NOT NULL" field) is undef to determine whether there was a matching row or not, and skip the second query if there's nothing to lock. 2) Although $ID did just come from the database, so it's probably safe, you really should use a placeholder in the second query instead of interpolating it into the query string. Mostly just for the sake of keeping up the habit of using placeholders whenever possible, but I also can't rule out the possibility that someone might be able to exploit a weird corner case to turn it into an SQL injection vulnerability. A quick (and completely untested) rewrite:
In Section
Seekers of Perl Wisdom
|
|