There's always more than one way to do it, but I think you're working harder here than you have to.

All your

if ($line =~...

...blocks are very similar.

I would probably suggest not using the heavy regular expressions as you are doing, but rather using the split operator. And I wouldn't treat the first token in the line any differently from the rest. I'd also be lazier than you about picking out each field by name. Something like this:

my ($type,@tokens)=split(/\~/,$line); if ($type eq "SendFiltered") { my ($tstamp,$cmnd_id,$alert_id,$send_id,$state,$svr_host) = @tokens[0-5]; my $sql = "INSERT ... } elsif ($type eq "AlertError") { ... }

However even that isn't really sufficiently lazy. The SQL is still there, and it's still ugly. (This isn't your fault - SQL is just plain ugly.)

It seems to me that since you're not doing anything at all with all of these values except sticking them in the SQL statements, you don't actually need to bother naming them at all. Really all you need to know for each "first token" type is what the SQL statement should be, and which column numbers to plug in.

With that in mind, you could do something like this:

# %statements is an array of prepared SQL statement objects ready to h +ave values plugged in %statements=( "AlertError"=>$dbh->prepare("INSERT INTO ERRORS (TSTAMP,CMND_ID,ALER +T_ID,SEND_ID,DESTINATION,MESSAGE,STATE,SVR_HOST) VALUES (?,?,?,?,?,?, +?,?)"), "SendCleared"=>$dbh->prepare("UPDATE SENDS..."), "... ); # %columns is an array of column position indicators corresponding to +the %statements above %columns=( "AlertError"=>[0,1,2,3,4,5], "SendCleared"=>[... );

Then your actual executable code becomes quite easy...

while ($line=...) { my ($type,@tokens)=split(/\~/,$line); $statements{$type}->execute(@tokens[@{$columns{$type}}]); }

There are various other ways you could organise the data, including using OOP. The key point is that the SQL statements and the column mappings really are data, not code, from the point of view of your Perl program.


In reply to Re: Need Critique (Good or Bad) by thparkth
in thread Need Critique (Good or Bad) by onegative

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":



  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.