in reply to For Review: DB Import

This looks like a good bit of code.

I would put in the OS respose at the open file line with $!, even tho' it may not be appropriate considering the user of the script, if there are any errors they can give back the exact details under $!.

Using a rather personal style (if perf isnt an issue) I like to stick my SQL into an OS file that I suck in. I like to do this because I can edit the sql directly, meaning I cant accidently modify the supporting code.

Another personal thing, I would perhaps issuse a challenge for the database authen, rather than store the pwd. It doesnt take a rocket scientist for a novice to use google, learn a few perly bits, and get back the pwd they could use in another app. If you must store the password, why not use a two way encryption engine (such as DES or 3DES) and store the key in a protected file somewhere. Not really what I consider brutely secure, but better than effectively keeping it in the clear.

You dont do any decent checking of the header/booth vars. I am not familiar with MySQL (nor an expert on the SQL 92 std) , however i would think it would lend itself to the old subquery trick where the column to be inserted is the result of a (sub)query itself. (also if a user accidently enters a ' (single quote) it will bugger up the query (well, it will with postgres and oracle))

If $booth_table doesnt change thru'out the code, why not:

  • use a constant
  • hardcode it?

    Personally, I like the constant, but it depends on your style.

    OK, so i'm being incredilby nit-picky, and anal retentive about it all. Above all, it seems like a good bit of code, easy to read and easy to maintain. Most of my suggestions are personal preference and could be just plainly be avoided.

    One thing I never avoid in my code that will be used by untrusted 3rd parties is logging. Grab an IP, or provide authen to use the script, but log every untrusted 3rd party that may use your script. It definately a CYA measure, but can solve disputes, uncover problems you may have with security/perf et al. IMHO very important.

    Another thing I would not avoid is checking the return of any DBI execute statements and handling the errors accordingly (ie to fit your context).

    Overall a good bit of code, perhaps an 7..8/10 by my standards, well done (++).

    Update:Thanks to Zaxo who pointed out that raiseerror is being called. I did notice that, however I meant some explicit error handling (altho it may not be needed to this type of app)

    Update2: In answer to impossiblerobot when i write apps like this and there is an OS error I usally dump some text letting the user know there is an error and ask them to cut and paste the output of $! into an email...
  • Replies are listed 'Best First'.
    Re: Re: For Review: DB Import
    by impossiblerobot (Deacon) on Jan 25, 2002 at 22:19 UTC
      Thanks, Ryszard. I agree at including $! could be more useful, even to an end-user. (I'm curious as to how most monks go about combining a user-friendly error message with the sometimes less friendly $! version.)

      Although I don't think this is a good application for putting the SQL in a separate file, it is definitely something I will think about for future projects.

      The obscured password is not for strict security; it's to prevent someone who looks over my shoulder, or to whom I show the code, from having the password burned into their brain accidentally.

      I like your idea of using a constant for variables like $booth_table that don't change (though I understand that perl optimizes for cases like this anyway, and it does make interpolation more complex). And I think logging would be perfect for this particular application, which often results in large changes in data.

      I think you're also dead-on about catching my DBI errors. Although I'm using RaiseError, it could use more user-friendly error-reporting/handling.

      Impossible Robot