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


This is a code for a search engine. I was wondering if this 
code can be improved for better performance. Can anyone
suggest where and how I can improve this code?

Thanks in Advance.


#!/usr/bin/perl

require "konnect1.pl"; 
require "cgilib.pl";

&readParse(*dict);

$index = 0;
%SearchResults = "";

$SearchResults{'keyterm'} = $dict{'Searchphrase'} || $dict{'Keyterm'};
$SearchResults{'fieldname'} = $dict{'Fieldname'};
$SearchResults{'lastcount'} = $dict{'Lastcount'} || 0;
$SearchResults{'pageno'} = $dict{'Page'} || 0;

ErrorPage() if ($SearchResults{'keyterm'} !~ /(\w+)/);


    # Retrieve the results based on keyterm
    $Query = qq(Select id, searchterm, magazinetitle, issueno, );
    $Query .= qq(startpage, endpage, buynow );
    $Query .= qq(FROM Database);
    $Query .= qq(WHERE );
       
         if ($SearchResults{'fieldname'} eq "Searchterm")
         {    $Query .= qq(searchterm like "%$SearchResults{'keyterm'}
+%" );  }
         else
         {    $Query .= qq(issueno = $SearchResults{'keyterm'} );   }

    $Query .= qq(ORDER BY searchterm, issueno);


    $sth = $dbh -> prepare($Query);
    $sth -> execute;
    while(@results = $sth -> fetchrow_array)
    {
          $index++;
          $SearchResults[$index]{'id'} = $results[0];
          $SearchResults[$index]{'searchterm'} = $results[1];
          $SearchResults[$index]{'magazinetitle'} = $results[2];
          $SearchResults[$index]{'issueno'} = $results[3];
          $SearchResults[$index]{'startpage'} = $results[4];
          $SearchResults[$index]{'endpage'} = $results[5];
          $SearchResults[$index]{'buynow'} = $results[6];
          $SearchResults[$index]{'index'} = $index;
          $SearchResults{'count'} += 1;
    }
    $sth -> finish;     

    ErrorPage() if (!$SearchResults[1]{'id'});
   
    $SearchResults{'quotient'} = int($SearchResults{'count'}/10);
    $SearchResults{'pages'} = $SearchResults{'quotient'} + 1;
 

    if (($SearchResults{'pageno'} != $SearchResults{'pages'} && $Searc
+hResults{'pageno'} != 0) ||                                          
+                                    ($SearchResults{'count'} == $Sear
+chResults{'lastcount'}))
    {
           $SearchResults{'end'} = ($SearchResults{'pageno'} != 0) ? $
+SearchResults{'pageno'}*10 : $SearchResults{'quotient'}*10;
           $SearchResults{'start'} = $SearchResults{'end'} - 9;
    }
    elsif ($dict{'Action'} eq "prev")
    {
           $SearchResults{'end'} = $SearchResults{'lastcount'} - 10;
           $SearchResults{'start'} = $SearchResults{'end'} - 9;
    }
    elsif (($SearchResults{'pageno'} == $SearchResults{'pages'}) || ((
+$SearchResults{'count'} - $SearchResults{'lastcount'}) < 10))
    {
           $SearchResults{'start'} = ($SearchResults{'pageno'} != 0) ?
+ ($SearchResults{'quotient'}*10)+1 : $SearchResults{'lastcount'}+1;
           $SearchResults{'end'} = $SearchResults{'count'};
    }
    else    #COMMENT: ($dict{'Action'} eq "next" || $dict{'Action'} eq
+ "")
    {
           $SearchResults{'start'} = $SearchResults{'lastcount'} + 1;
           $SearchResults{'end'} = $SearchResults{'lastcount'} + 10;
    }


    HtmlBegin($SearchResults);
    $SearchResults{'lastcount'} = HtmlMiddle($SearchResults);

    if ($SearchResults{'count'} < 10)
    {      HtmlEnd($SearchResults, $Next = 0, $Previous = 0);  }
    elsif ($SearchResults{'count'} > 10 && $SearchResults{'lastcount'}
+ == 10) 
    {      HtmlEnd($SearchResults, $Next = 1, $Previous = 0);  }
    elsif ($SearchResults{'count'} == $SearchResults{'lastcount'})
    {      HtmlEnd($SearchResults, $Next = 0, $Previous = 1);  }
    else
    {      HtmlEnd($SearchResults, $Next = 1, $Previous = 1);  }
    
#~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~S U B R O
+ U T I N E S~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


sub HtmlBegin
{
     $SearchResults = $_ if $_;

     print "Content-type: text/html\n\n";

     open (TOP, "front_table_top.asp");
     while (<TOP>)
     {  print $_; }

     print <<EOF;
          <script language="JavaScript">
            <!-- function MM_openBrWindow(theURL,winName,features) { /
+/v2.0 window.open(theURL,winName,features); } // -->
          </script>

          <!--  -->

        </header>

        <body bgcolor="#000000">
         <center>
            <h2><b><font color="#FFFF00">Search Results</font></b></h2
+>
          </center><br><br>
            <font size="2" color="#FFFF00" face="arial,helvetica,sans-
+serif">Searchterm is <b>$SearchResults{'keyterm'}</b></font><br>
            <p>
            <dl>
EOF
}
   


sub HtmlMiddle
{
    $SearchResults = $_ if $_;

    for ($index = $SearchResults{'start'}; $index <= $SearchResults{'e
+nd'}; $index++)
    {
          $printHypertextLink = qq(<dd><h3><font color="#FFFFFF"><a hr
+ef="http://eclipse.vcix.com/cgi-bin/);
          $printHypertextLink .= qq(Starlog.storefront/EN/product/$Sea
+rchResults[$index]{'buynow'}" ); 
          $printHypertextLink .= qq(target=_blank>$SearchResults[$inde
+x]{'magazinetitle'} );
          $printHypertextLink .= qq(#$SearchResults[$index]{'issueno'}
+        );

          if ($SearchResults[$index]{'startpage'} == $SearchResults[$i
+ndex]{'endpage'})
          {   $printHypertextLink .= qq(page(s): $SearchResults[$index
+]{'startpage'}</a></font></h3><br>); }
          else
          {  
              $printHypertextLink .= qq(page(s): $SearchResults[$index
+]{'startpage'}-);
              $printHypertextLink .= qq($SearchResults[$index]{'endpag
+e'}</a></font></h3><br>); 
          }
               


          if ($SearchResults[$index]{'searchterm'} eq $temp)
          {
              print $printHypertextLink;
          }
          else
          {
              print "<dt><h2><b><font color=\"maroon\">$SearchResults[
+$index]{'searchterm'}</font></b></h2>";
              print $printHypertextLink;
          }

          $temp = $SearchResults[$index]{'searchterm'};
    }

    return($SearchResults[$index - 1]{'index'});
}




sub HtmlEnd
{
      ($SearchResults, $Next, $Previous) = @_ if @_; 


      $PreviousImage = "<input type=\"submit\" name=\"Action\" value=\
+"prev\">" if ($Previous);
      $NextImage = "<input type=\"submit\" name=\"Action\" value=\"nex
+t\">" if ($Next);



      print <<EOF;
        </dl>
          <br>
          <br>
          <hr width="100%" align="center">
        
          <form method="POST" action="SearchEngine.pl">
          <input type="hidden" name="Keyterm" value=$SearchResults{'ke
+yterm'}>
          <input type="hidden" name="Lastcount" value=$SearchResults{'
+lastcount'}>
          <input type="hidden" name="Fieldname" value=$SearchResults{'
+fieldname'}>
           
          <table width="100%" border="0" align="center">
            <tr align="center" valign="middle">
               <td width="5%">$PreviousImage</td>
               <td width="*"><font face="Arial, Hfelvetica, sans-serif
+" color="#FFFFFF">There are $SearchResults{'count'} matching titles.<
+br />
                   Displaying matches:  $SearchResults{'start'} - $Sea
+rchResults{'end'}</font></td>
               <td width="5%">$NextImage</td>
            </tr>
          </table>

          <hr width="100%" align="center">
          </center>
         </form>
         <center>
EOF

      
      $SearchResults{'temppgno'} = ($SearchResults{'pageno'} != 0) ? $
+SearchResults{'pageno'} : int($SearchResults{'lastcount'}/10);

      for ($i = 1; $i <= $SearchResults{'pages'}; $i++)
      {
           $Link = qq(<a href="SearchEngine.pl?);
           $Link .= qq(Keyterm=$SearchResults{'keyterm'}&Lastcount=$Se
+archResults{'lastcount'}&);
           $Link .= qq(Fieldname=$SearchResults{'fieldname'}&Page=$i>)
+;
           $Link .= qq(<font face="Arial, Hfelvetica, sans-serif" colo
+r="yellow">[$i]</font></a>);

           if ($i%10 == 1)
           {   print "<br />\n";  }

           if ($i == int($SearchResults{'temppgno'}))
           {   print qq(<font face="Arial, Hfelvetica, sans-serif" col
+or="white">[$i]</font>);  }
           else
           {   print $Link;  }

           print "&nbsp;&nbsp;";
      }

      open (BOTTOM, "front_table_bottom.asp");
      while (<BOTTOM>)
      {  print $_; }

exit;
}




sub ErrorPage
{
      print "Content-type: text/html\n\n";

      open (TOP, "front_table_top.asp");
      while (<TOP>)
      {  print $_; }

            print  "<h2>No Match Found</h2>";

      open (BOTTOM, "front_table_bottom.asp");
      while (<BOTTOM>)
      {  print $_; }

exit;
}

[download]

Replies are listed 'Best First'.
RE: Help required to improve code
by Petruchio (Vicar) on Oct 05, 2000 at 16:48 UTC
    Upon looking at this code further, I have a few suggestions.

    First, you must tidy it up. Really. It'll save you pain. The reason I was able to see how to get rid of the

    while(@results = $sth -> fetchrow_array)

    loop is because it's by far the neatest part of your code. Make your variable names shorter (especially ones which don't travel far, like a loop counter), and make your indentation very regular so that this:

    $SearchResults{'keyterm'} = $dict{'Searchphrase'} || $dict{'Keyterm'};
  +
  $SearchResults{'fieldname'} = $dict{'Fieldname'};
  $SearchResults{'lastcount'} = $dict{'Lastcount'} || 0;
  $SearchResults{'pageno'} = $dict{'Page'} || 0;

    [download]

    looks like this:

    $srchRes{'keyterm'}   = $dict{'Searchphrase'} || $dict{'Keyterm'};
$srchRes{'fieldname'} = $dict{'Fieldname'};
$srchRes{'lastcount'} = $dict{'Lastcount'}    || 0;
$srchRes{'pageno'}    = $dict{'Page'}         || 0;

    [download]

    Believe me, I don't say this because I'm any kind of neat freak. It will make your life much, much easier.

    Once you've done this, look at your loops. If you're saying something like:

    if ($a){
  $b;
  $c;
}
else{
  $b;
  $d;
}

    [download]

    You should rewrite it to say:

    $b;
if ($a){
  $c;
}
else{
  $d;
}

    [download]

    This happens more than once in your code...

    if ($SearchResults[$index]{'startpage'} == $SearchResults[$index]{'end
+page'})
            {   $printHypertextLink .= qq(page(s): $SearchResults[$ind
+ex]{'startpage'}</a></font></h3><br>); }
            else
            {  
                $printHypertextLink .= qq(page(s): $SearchResults[$ind
+ex]{'startpage'}-);
                $printHypertextLink .= qq($SearchResults[$index]{'endp
+age'}</a></font></h3><br>);

    [download]

    should look more like:

    $printHL   .= qq/page(s): $srchRes[$i]{'startpage'}/;

unless ($srchRes[$i]{'startpage'} == $srchRes[$i]{'endpage'}){
  $printHL .= qq(-$srchRes[$i]{'endpage'}); 
}

$printHL   .= qq(</a></font></h3><br>);

    [download]

    Same goes for your loops... you don't need to build the same literal string every time you iterate through a for loop.

    And then... there are your variables. All of your variables are global. If your script is worth optimizing... if it's worth keeping... do something about this. Certain parts of your script are NOT doing what you think they are. Besides, it's very difficult to change anything without breaking the whole script, when any change you make will affect the whole script. Study up on the my keyword. Your sanity may depend on it.

[reply]
[d/l]
[select]
Re: Help required to improve code
by Jouke (Curate) on Oct 05, 2000 at 09:45 UTC
[reply]
Re (tilly) 1: Help required to improve code
by tilly (Archbishop) on Oct 05, 2000 at 15:06 UTC
    I would start by saying that you should get familiar with strict and my. Walk through some random tutorials, see if you can find other things to clean up. For instance while reading from the database you could try something like this: 
    while (my $row = $sth->fetchrow_hashref) {
    push @SearchResults, $row;
}

    [download]
    Does the same thing you are already doing, but it is faster, easier to read, less room for error.

    In general though, your code is at the point where you need to work more on pushing the work down to Perl, factoring out common strings, etc. The biggest win is in readability, and maintainability, but generally performance will improve as well.

[reply]
[d/l]
Re: Help required to improve code
by PotPieMan (Hermit) on Oct 05, 2000 at 17:09 UTC

    It may be a moot point, but I think it would be advisable to move over to CGI.pm as opposed to cgi-lib.pl. The functionality is pretty much the same, and (I think) the query parameters are easier to access. Any programmer who might later look at your code will expect CGI.pm. All in all, I guess it all depends on whether or not you want to spend the time porting to CGI.pm.

    As an interesting side note, Tom Christiansen wrote a little piece in 1996 about using CGI.pm instead of cgi-lib.pl. A lot of it doesn't really apply now, but it's somewhat amusing to read now.

    -ppm

[reply]
Re: Help required to improve code
by cadfael (Friar) on Oct 05, 2000 at 17:42 UTC
    One feature of CGI.pm is that you can use the :cgi-lib function set to retain compatibility with cgi-lib.pl.

    I don't know whether using this will speed up anything, but it may make the job of upgrading to more modern CGI methods a little easier.

    I also evolved from Perl 4 to Perl 5, and from older ways of doing things to newer ways. Rewriting a lot of scripts is never fun, but once a workable template is in place, new scripts are a whole lot easier.

[reply]
Re: Help required to improve code
by Anonymous Monk on Oct 05, 2000 at 10:28 UTC
    First impression, since your hash keys are the same as your column names, you might want to replace

    while(@results = $sth -> fetchrow_array)
{
  $index++;
  $SearchResults[$index]{'id'} = $results[0];
  $SearchResults[$index]{'searchterm'} = $results[1];
  $SearchResults[$index]{'magazinetitle'} = $results[2];
  $SearchResults[$index]{'issueno'} = $results[3];
  $SearchResults[$index]{'startpage'} = $results[4];
  $SearchResults[$index]{'endpage'} = $results[5];
  $SearchResults[$index]{'buynow'} = $results[6];
  $SearchResults[$index]{'index'} = $index;
  $SearchResults{'count'} += 1;
}

    [download]

    with

    {
  my $i = {};
  @SearchResults = @{$sth->fetchall_arrayref($i)};
  $SearchResults{count} = ++$#SearchResults;
}

    [download]

    ...just 'cuz I'm guessing DBI's faster than you are. ;-)

    Looks like you could hoist a bunch of stuff out of those loops in your subroutines, too. But as I see ".asp" which stands for "Ain't Stuff for Petruchio", I'll leave that to the smarter monks... many of whom will doubtless tell you to use strict, by the way.

[reply]
[d/l]
[select]
      Dag nab it! Posted anonymously... oh, well... who needs XP anyway? :-)
[reply]
Re: Help required to improve code
by AgentM (Curate) on Oct 05, 2000 at 19:02 UTC
    Rule #1: perl -wT,use strict, use CGI; This will never hurt you and cna only serve to safeguard your CGI. Turn off POST uploads and set POST_MAX.
    For a needed speed improvement, check out mod_perl or FastCGI which will boost your performance by caching your CGI.
    Tip #3: While you're using DBi, you might as well use the "?" which allow you throw in data along the way in your query. In this manner, you could probably conactenate your entire query mess into one or two lines. If you want a real speed improvement, you can read from the dbengine one at a time, see the DBI docs. While this eats up the same memory in the dbengine, the user will see results more quickly which is probably what you want. That way, you can also store count up the first ten and stop automatically instead of reading in the first ten and then displaying them. Anyway, your SQL statement is lousy since it requests all statements, displays the first select few and doesn't even bother to cache the next few. Most dbengines have support for "get me the first ten matches" or something like that. If it doesn't a trick may be to read in all of the matches (simultaneously displaying them), cache them (minimally in timed-out tmp file but better using FastCGI) and the next next theuser comes around, he has his data waiting for him. Currently, you are wasting alot of user time just reading in data from the dbegine that you read only seconds before! As your site grows, the decrease in dbengine performance will be VERY visible. The brand new CGI::Cache is also a nifty module that i would strongly recommend for integration into your script. If you're interested in further reading into the FastCGI/mod_perl issues, click here. (**for some reason, fastcgi.com seems to be down at the current time 11amEST10-5-00 so check it out perhaps a bit later.) But whatever you do take into consideration and integration, it will take some time. Some of your script needs some big rewrites. And as I and tilly were discussing before, this is "real problem" (i.e. nothing to ignore- security, performance, resource waste). If you'd rather not write it yourself, do a search for "DBI search engine" at a few sites and take the optimized code that we've been discussing. One more thing (I know I'm long-winded)- you should use the -compile option in your use CGI qq/.../; statement. This will precompile most of your HTML prints so you end up with something even more decent.
    AgentM Systems or Nasca Enterprises is not responsible for the comments made by AgentM- anywhere.
[reply]
[d/l]
[select]

Back to Seekers of Perl Wisdom