Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:

Hi Monks!
I have this as part of my Perl program where I need to have these SQL queries done several times, I just don’t like how repetitive this code is, I can’t think of a better and more efficient way of doing this. Could there be a way to optimize the “connect” part at least or may be a more elegant way of doing this part of the code?

Thanks a lot! 

my $db = "MyServer";
my $user = MyServerStuff::odbc->{ $db } { user };
my $pass = MyServerStuff::odbc->{ $db } { pass };


my $dbh = DBI->connect("DBI:ODBC:$db",$user, $pass, {RaiseError => 1})
+;
my $dbh_b = DBI->connect("DBI:ODBC:$db",$user, $pass, {RaiseError => 1
+});
my $dbh2 = DBI->connect("DBI:ODBC:$db",$user, $pass, {RaiseError => 1}
+);
my $dbh2_b = DBI->connect("DBI:ODBC:$db",$user, $pass, {RaiseError => 
+1});
my $dbh3 = DBI->connect("DBI:ODBC:$db",$user, $pass, {RaiseError => 1}
+);
my $dbh3_b = DBI->connect("DBI:ODBC:$db",$user, $pass, {RaiseError => 
+1});
my $dbh4 = DBI->connect("DBI:ODBC:$db",$user, $pass, {RaiseError => 1}
+);
my $dbh4_b = DBI->connect("DBI:ODBC:$db",$user, $pass, {RaiseError => 
+1});
my $dbh5 = DBI->connect("DBI:ODBC:$db",$user, $pass, {RaiseError => 1}
+);
my $dbh5_b = DBI->connect("DBI:ODBC:$db",$user, $pass, {RaiseError => 
+1});


my $sql_s="select distinct main.type,
                                           history.date_forwarded,COUN
+T(1) as 'PPending'
          from main,history
                  where main.type='1'
                  and history.date_forwarded
                  between dateadd( day, -7, getdate()) and getdate()
                  group by history.date_forwarded,main.type";
                  
my $sql_sb="select distinct main.type,
                                             history.date_forwarded,CO
+UNT(1) as 'PPending'
                    from main,history
                    where main.type!='1'
                    and history.date_forwarded
                    between dateadd( day, -7, getdate()) and getdate()
                    group by history.date_forwarded,main.type";
                                                                      
+                                                            ";
my $sql_s2="select status_now, COUNT(1) as 'Got Total'
                    from main
                    where status_now='submitted' and type='1'
                    and submitted_date
                    between dateadd( day, -7, getdate()) and getdate()
                    group by status_now";
                    
my $sql_s2b="select status_now, COUNT(1) as 'Got Total'
                     from main
                    where status_now='submitted'and type!='1'
             and submitted_date
                     between dateadd( day, -7, getdate()) and getdate(
+)
                     group by status_now";
                     
my $sql_s3="select status_now, COUNT(1) as 'All_Completed'
                   from main
                   where status_now='completed'and type='1'
                   and submitted_date
               between dateadd( day, -7, getdate()) and getdate()
                    group by status_now";
                    
my $sql_s3b="select status_now, COUNT(1) as 'All_Completed'
                     from main
                     where status_now='completed'and type!='1'
                     and submitted_date
                between dateadd( day, -7, getdate()) and getdate()
                    group by status_now";
                    
my $sql_s4="select delete_check, COUNT(1) as 'Total Deleted'
                   from main
                   where delete_check='yes'and type='1'
                   and submitted_date
                   between dateadd( day, -7, getdate()) and getdate()
                   group by delete_check";
                   
my $sql_s4b="select delete_check, COUNT(1) as 'Total Deleted'
                     from main
                     where delete_check='yes'and type!='1'
                     and submitted_date
                     between dateadd( day, -7, getdate()) and getdate(
+)
                     group by delete_check";
                     
my $sql_s5="select status_now, COUNT(1) as 'Total Pending'
                    from main
                    where status_now='forwarded' and type='1'
                    and submitted_date
                    between dateadd( day, -7, getdate()) and getdate()
                    group by status_now";
                    
my $sql_s5b="select status_now, COUNT(1) as 'Total Pending'
                      from main
                      where status_now='forwarded' and type!='1'
                      and submitted_date
                      between dateadd( day, -7, getdate()) and getdate
+()
                      group by status_now";

my $sth = $dbh->prepare($sql_s);
$sth->execute() || die $sth->errstr;

my $sth_b = $dbh_b->prepare($sql_sb);
$sth_b->execute() || die $sth_b->errstr;

my $sth2 = $dbh2->prepare($sql_s2);
$sth2->execute() || die $sth2->errstr;

my $sth2_b = $dbh2_b->prepare($sql_s2b);
$sth2_b->execute() || die $sth2_b->errstr;

my $sth3 = $dbh3->prepare($sql_s3);
$sth3->execute() || die $sth3->errstr;

my $sth3_b = $dbh3_b->prepare($sql_s3b);
$sth3_b->execute() || die $sth3_b->errstr;

my $sth4 = $dbh4->prepare($sql_s4);
$sth4->execute() || die $sth4->errstr;

my $sth4_b = $dbh4_b->prepare($sql_s4b);
$sth4_b->execute() || die $sth4_b->errstr;

my $sth5 = $dbh5->prepare($sql_s5);
$sth5->execute() || die $sth5->errstr;

my $sth5_b = $dbh5_b->prepare($sql_s5b);
$sth5_b->execute() || die $sth5_b->errstr;


while ($pointer = $sth->fetchrow_hashref)
  {
                  $date_forwarded = $pointer->{'date_forwarded'};$date
+_forwarded=~s/\s+$//g;
                  $miss_request = $pointer->{'PPending'}; $miss_reques
+t=~s/\s+$//g;
                  push @g_total, $date_forwarded;
}


while ($pointer = $sth_b->fetchrow_hashref)
{
                     $c_date_forwarded = $pointer->{'date_forwarded'};
+$c_date_forwarded=~s/\s+$//g;
                     $c_pend_request = $pointer->{'PPending'}; $c_pend
+_request=~s/\s+$//g;
                     push @c_g_total, $c_date_forwarded;
}

while ($pointer = $sth2->fetchrow_hashref)
{
                     $status_now = $pointer->{'status_now'};$status_no
+w=~s/\s+$//g;
                     $total_received = $pointer->{'Got Total'};$total_
+received=~s/\s+$//g;
                     push @total_pending,$total_received;
}

while ($pointer = $sth2_b->fetchrow_hashref)
{
                     $c_status_now = $pointer->{'status_now'};$c_statu
+s_now=~s/\s+$//g;
                     $c_total_received = $pointer->{'Got Total'};$c_to
+tal_received=~s/\s+$//g;
                     push @c_total_pending,$c_total_received;
}


while ($pointer = $sth3->fetchrow_hashref)
{
                     $comp_status_now = $pointer->{'All_Completed'};$s
+tatus_now=~s/\s+$//g;
                     push @comp_status_now, $comp_status_now;
}

while ($pointer = $sth3_b->fetchrow_hashref)
{
                     $c_comp_status_now = $pointer->{'All_Completed'};
+$c_status_now=~s/\s+$//g;
                     push @c_comp_status_now, $c_comp_status_now;
}

while ($pointer = $sth4->fetchrow_hashref)
{
                     $delete_check = $pointer->{'Total Deleted'};$dele
+te_check=~s/\s+$//g;
                     push @delete_check, $delete_check;
}

while ($pointer = $sth4_b->fetchrow_hashref)
{
                     $c_delete_check = $pointer->{'Total Deleted'};$c_
+delete_check=~s/\s+$//g;
                     push @c_delete_check, $c_delete_check;
}

while ($pointer = $sth5->fetchrow_hashref)
{
                     $total_pending_status_now = $pointer->{'Total Pen
+ding'};$total_pending_status_now=~s/\s+$//g;
                     push @total_pending_status_now, $total_pending_st
+atus_now;
}

while ($pointer = $sth5_b->fetchrow_hashref)
{
                     $c_total_pending_status_now = $pointer->{'Total P
+ending'};$c_total_pending_status_now=~s/\s+$//g;
                     push @c_total_pending_status_now, $c_total_pendin
+g_status_now;
}

[download]

Replies are listed 'Best First'.
Re: Efficiency on Perl Code!
by gam3 (Curate) on Oct 05, 2007 at 15:50 UTC
    I have rewritten your code in a form that I like. I am not sure that it will meet your needs, but it will show you some techniques that you can use.

    As noted by others, I am only using one database handle.

    Then I moved your SQL into a data structure and since you seem to only be using 1 data element for each select I gave them all the name data so that I can simplify the read loop. Look for as data in the SQL.

    And put the output in to another data structure.

    I also use use strict and catch any errors that DBI should happen to through.

    To extend this you can add information to the SQL data structure and even put a subroutine in it to handle each request.

    Read more... (4 kB)
    If you need to access the code again you do not need to prepare the data again. You can just run the second for loop.
    -- gam3
    A picture is worth a thousand words, but takes 200K.
[reply]
[d/l]
      I like that!!!
[reply]
Re: Efficiency on Perl Code!
by dragonchild (Archbishop) on Oct 05, 2007 at 15:23 UTC
    First off, you're new to programming. So, you should go ahead and read some basic books on programming. If Perl is going to be a major focus, I would strongly recommend Learning Perl, aka the Llama book by Randal Schwartz.
    • All your $dbhs are going against the same DB. So, just use one $dbh.
    • Whenever you see yourself naming a lot of variables almost the same, use a hash or an array.
    • Whenever you have repeated blocks of code that vary just by the name of a variable, use a subroutine or a loop.

    Update: Changed the name of the book from the Apress book to the O'Reilly book. Whoops!

    My criteria for good software:
    1. Does it work?
    2. Can someone else come in, make a change, and be reasonably certain no bugs were introduced?
[reply]
    A reply falls below the community's threshold of quality. You may see it by logging in.
Re: Efficiency on Perl Code!
by throop (Chaplain) on Oct 05, 2007 at 15:51 UTC
    Gentle monk,

    Yes, the degree of repetition is a red flag that you're working inefficiently. One of your problems is that you have a sequence of variables that aren't (programatically) connected to each other, even though their names form a series 

    my $dbh = ...
my $dbh_b = ...
my $dbh2 = ...
my $dbh2_b = ...

    [download]
    Group these all together into a hash over which you can iterate. 
    use strict;

my $db = "MyServer"; my $user = MyServerStuff::odbc->{ $db } { user };
+ my $pass = MyServerStuff::odbc->{ $db } { pass };

my %dbh;
for(my $ix= 1; $ix < 6; $ix++){
  foreach my $choice (qw(a b)){
    $dbh{$ix}{$choice} = DBI->connect("DBI:ODBC:$db",$user, $pass, {Ra
+iseError => 1})}};

    [download]
    For the part that generates the call to the database, wrap the code in a similar loop. Use sprintf to interpolate the '!' that switches off and on, and the varying 'from main / where main ' clauses.

    throop

    Updated: started $ix at 1
[reply]
[d/l]
[select]
      I don't understand this part "foreach my $choice (qw(a b)){" , I am running this code and its giving me the double lines in the for loop, "for(my $ix= 0; $ix < 6; $ix++){ " it is printing 12 lines instead of 6.
[reply]
[d/l]
[select]
        I've updated my code for $ix to start at 1, not 0. You had 10 variables in your opening block – dbh (1 thru 5) and dbh_b (1 thru 5). This loop generated both sets.

        throop

[reply]
Re: Efficiency on Perl Code!
by mr_mischief (Monsignor) on Oct 05, 2007 at 14:55 UTC
    Are you connecting to more than one database? If not, you can just connect once and prepare multiple statement handles on the one database handle. In fact, if your statement handles need not overlap, you could re-use your $sth lexical for new statements, too.
[reply]
      In fact, if your statement handles need not overlap, you could re-use your $sth lexical for new statements, too.

      That's the wrong kind of efficiency. Scalars are cheap. Execution plans are expensive. It would be better to connect to the database (there appears to be only one), prepare all the different queries into separate statement handles (possibly in an array), and then sit in a loop forever and spin on them (with a sleep at the end of the loop), as it looks like the code is monitoring a flow, rather than a single one-off affair.

      A lot of the queries look very similar. I would be tempted to union them all together to avoid thunking between to the database and Perl too much. Get it all in one hit, and then pull it apart in Perl-space afterwards.

      Instead of trimming the trailing blanks off in Perl, format the data correctly in the first place in the select so you don't have to fiddle with it afterwards.

      Finally, are these statements returning more than one row? If they are returning only a single row, there are better ways of processing such result sets other than a while/fetchrow_hashref (which is just about the worst way of processing a result set).

      • another intruder with the mooring in the heart of the Perl

[reply]
        I didn't mention reusing $sth for reasons of execution efficiency. Some people find it more straightforward to reuse a scalar for a particular kind of resource in non-overlapping situations.

        One could have a block scope around $sth and reuse the name as different lexical variables. One could simply reuse it. There could be an array of statement handles that's pushed and popped like a stack in some situations, or that treats the array as a set of statement handles that are all visible at once in other situations.

        It's not a matter of only having one scalar for Perl to keep track of, but of only having to keep track of as many scalars in one's own head as there are active handles of that type. If I can reuse $sth, I know the one to fetch from is $sth. If not, is it $sth[23] or $sth[24] that holds the particular statment handle from which I need to fetch in this line?

        I know some people don't think this way, but it's a decision that some people make. If you'd rather have a separate variable for each handle, go ahead. If it's easier for you to only keep track of as many as are actually in use, then I think that's a valid approach as well.

[reply]
[d/l]
[select]
      A reply falls below the community's threshold of quality. You may see it by logging in.
      On the other hand if the OP is connecting to multiple databases (on different servers), it might pay off to use threads to parallelize the db requests.

      Perl 6 in German

[reply]
Re: Efficiency on Perl Code!
by apl (Monsignor) on Oct 05, 2007 at 15:40 UTC
    Make your various $sql definitions into an array.

    Make an array the has $pointer will use.

    Make an array of references to the arrays you'll push.

    The you have (with lots of room for exercises for the user)
    my $dbh = DBI->connect("DBI:ODBC:$db",$user,$pass,{RaiseError => 1});
my $sth;
my @sql = .....;
my @ptr = .....;
my @arr = .....;

for my $idx ( 0..$#sql )
  {
    $sth = $dbh->prepare( $sql[$idx] );
    $sth->execute() || die $sth->errstr;

    while ($pointer = $sth->fetchrow_hashref)
      {
        $value = $pointer->$ptr[$idx];
        $value =~ s/\s+$//g;
        push $arr[$idx], $value;
      }
  }

    [download]

    One array of a structure would be better than several arrays, but yoiu get the idea.
[reply]
[d/l]

Back to Seekers of Perl Wisdom