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

Greetings,fellas.

I'm in deep need of enlightening here. I'm writing a report script that should generate values monthly and then bring totals by trimesters.

The thing is I'm doing something wrong on the loops that is adding more to each trimester's total that it should. I've tried many things to solve it, but I'm failing and failing.

What I need here is more a theorical than practical explanation, I think.

Sorry by the large piece of code, but I think it is vital to understand the whole thing.
Edited by boo_radley : READMORE added.


Here's it: 
sub acumulado_f #I've called a sub 'cause there's more than one type o
+f report
{    
    $titulo = "Report by state";
    
    $dbh = SysUtils::Connect;
    
    $sthf = $dbh->prepare_cached("SELECT uf FROM filiais ORDER BY uf")
+; #selects each state
    
    $xf = 0;
    
    $sthf->execute;
    
    while (@dataf = $sthf->fetchrow_array())
    {
        $filiais[$xf] = $dataf[0];
        $xf++;
    }
    
    foreach $filiais(@filiais)#foreach state
    {
        $cnt =0;
        $cnt2 = 0;
        
        if ($cnt2 eq 0)# if it's the first result, assemble a header
        {
            $message .= "<BR><TABLE WIDTH=\"100%\" BORDER=1>
            <TR class=\"texto3\">
            <TD colspan=\"9\" BGCOLOR=\"#FFFFFF\"><span class =\"texto
+2\">State: $filiais</span></TD>
            </TR>
            <TR>
            <TD WIDTH=\"10%\">&nbsp;</TD>
            <TD COLSPAN=\"4\"  class=\"texto2\" align=\"center\">Incom
+e</TD>
            <TD COLSPAN=\"4\"  class=\"texto2\" align=\"center\">Margi
+n</TD>
            </TR>
            <TR>
            <TD WIDTH=\"10%\" class=\"texto2\">&nbsp;</TD>
            <TD WIDTH=\"13%\" ALIGN=\"CENTER\" class=\"texto2\">Predic
+ted</TD>
            <TD WIDTH=\"13%\" ALIGN=\"CENTER\" class=\"texto2\">Realiz
+ed</TD>
            <TD WIDTH=\"5%\" ALIGN=\"CENTER\" class=\"texto2\">%</TD>
            <TD WIDTH=\"13%\" ALIGN=\"CENTER\" class=\"texto2\">Differ
+ence</TD>
            <TD WIDTH=\"13%\" ALIGN=\"CENTER\" class=\"texto2\">Predic
+ted</TD>
            <TD WIDTH=\"13%\" ALIGN=\"CENTER\" class=\"texto2\">Realiz
+ed</TD>
            <TD WIDTH=\"5%\" ALIGN=\"CENTER\" class=\"texto2\">%</TD>
            <TD WIDTH=\"14%\" ALIGN=\"CENTER\" class=\"texto2\">Differ
+ence</TD>
            </TR>";
            
            $cnt2++;
        }
        
        for ($m=1; $m<=12; $m++)#Make an array of months
        {
            $months[$m] = $m;
        }
        
        foreach $months(@months)
        {
            if ($months ne "")#escapes the obligatory 0 position of th
+e array - blame me later for this one :)
            {
                $sh_mes = SysUtils::Extenso_Mes("$months");#Return the
+ month name
                $fat = 0;
                $mar = 0;
# In the database there are fields with the same number as months:
                $fa = "A.f".$months;
                $ma = "A.m".$months;
                $rf = "A.rf".$months;
                $rm = "A.rm".$months;
                
                $tf = 0; # First predicted value
                $trf = 0;# First realized value
                $tm = 0;# Second Predicted value
                $trm = 0;# Second realized value
                
                $ctp = 0;
               
                $sthn = $dbh->prepare_cached("SELECT DISTINCT ".$fa." 
+, ".$ma." , ".$rf." , ".$rm." FROM metas A, usuarios B WHERE B.filial
+='$filiais' AND A.login=B.login") or die "Preparing error. ".$dbh->er
+rstr;
                $sthn->execute or die "executing error. ".$sthn->errst
+r; #This statement works fine. I've debugged it and tested it
                
                while (@rown = $sthn->fetchrow_array())
                {
                    $fat = $rown[0];
                    $mar = $rown[1];
                    $rfat = $rown[2];
                    $rmar = $rown[3];
                    
                    $tf += $fat; #First predicted value
                    $tm += $mar; #Second predicted value
                    
                    $trf += $rfat;#First realized value
                    $trm += $rmar;#Second realized value
                    
                    $ctp++;
                    
                    if ($months eq "1") #January, obviously
                    {
                        $tft = 0; #trimester predicted 1st total
                        $trft = 0; #trimester realized 1st total
                        $tmt = 0; #trimester predicted 2nd total
                        $trmt = 0; #trimester realized 2nd total
                        
                        $tft += $tf;
                        $trft += $trf;
                        
                        $tmt += $tm;
                        $trmt += $trm;
                    }
                    elsif ($months eq "2" or $months eq "3")
                    {
                        $tft += $tf;
                        $trft += $trf;
                        
                        $tmt += $tm;
                        $trmt += $trm;
                    }
                    elsif ($months eq "4")
                    {
                        $tft = 0;
                        $trft = 0;
                        $tmt = 0;
                        $trmt = 0;
                        
                        $tft += $tf;
                        $trft += $trf;
                        
                        $tmt += $tm;
                        $trmt += $trm;
                    }
                    elsif ($months eq "5" or $months eq "6")
                    {
                        $tft += $tf;
                        $trft += $trf;
                        
                        $tmt += $tm;
                        $trmt += $trm;
                    }
                    elsif ($months eq "7")
                    {
                        $tft = 0;
                        $trft = 0;
                        $tmt = 0;
                        $trmt = 0;
                        
                        $tft += $tf;
                        $trft += $trf;
                        
                        $tmt += $tm;
                        $trmt += $trm;
                    }
                    elsif ($months eq "8" or $months eq "9")
                    {
                        $tft += $tf;
                        $trft += $trf;
                        
                        $tmt += $tm;
                        $trmt += $trm;
                    }
                    elsif ($months eq "10")
                    {
                        $tft = 0;
                        $trft = 0;
                        $tmt = 0;
                        $trmt = 0;
                        
                        $tft += $tf;
                        $trft += $trf;
                        
                        $tmt += $tm;
                        $trmt += $trm;
                    }
                    elsif ($months eq "11" or $months eq "12")
                    {
                        $tft += $tf;
                        $trft += $trf;
                        
                        $tmt += $tm;
                        $trmt += $trm;
                    }
                }
                
                $sthn->finish;
                
                if ($ctp ne 0)
                {
                    ListaDados(); #brings value for each month
                    
                    if ($months eq "3")# Is it a trimester?
                    {
                        $trim = 1;
                        ListaTrimestre(); #bring values (totals) for e
+ach trimester 
                    }
                    elsif ($months eq "6")
                    {
                        $trim = 2;
                        ListaTrimestre();
                    }
                    elsif ($months eq "9")
                    {
                        $trim = 3;
                        ListaTrimestre();
                    }
                    elsif ($months eq "12")
                    {
                        $trim = 4;
                        ListaTrimestre();
                    }
                }
                else
                {
                    if ($cnt eq 0)#No values to show
                    {
                        $message .= "<TR CLASS=\"texto3\">
                        <TD BGCOLOR=\"$color2\" colspan=\"9\">State ha
+s no values.</TD>
                        </TR>";
                    }
                    $cnt++;
                }
            }
        }
    }
    
    $dbh->disconnect;
    
#open template to show values.

    print "Content-type: text/html\r\n\r\n";
    open (FH,"templates/relatorios/relatorio_andamento.htm") or die "C
+ant open body: $!";
    $body = join('',<FH>);
    close (FH);
    eval "\$body = sprintf \"%s\",<<FYNYS;\n $body\nFYNYS\n";
    print $body;
}

[download]
The "ListaTrimestre" sub is a simple "put the vars into HTML code", so it displays later in the template. It is the $tft, $trft, $tmt and $trmt vars that are wrong. The problem is that I didn't found a pattern of error, so I acn't find where is the value that is being incorrectly added to the totals.

Can anyone point me the logic error here?

Thanks in advance,

Er Galvão Abbott
 a.k.a. Lobo, DaWolf
Webdeveloper

Replies are listed 'Best First'.
Re: Logic problem on loops
by chromatic (Archbishop) on Apr 21, 2002 at 20:18 UTC
    No logic errors stand out immediately, but you're doing way too much work. For example, 
    $xf = 0;
    
$sthf->execute;
    
while (@dataf = $sthf->fetchrow_array())
{
    $filiais[$xf] = $dataf[0];
    $xf++;
}

    [download]
    is generally better written as: 
    while (my ($col) = $sthf->fetchrow_array())
{
    push @filiais, $col;
}

    [download]
    Similarly, 
     for ($m=1; $m<=12; $m++)#Make an array of months
        {
            $months[$m] = $m;
        }
        
        foreach $months(@months)
        {

    [download]
    would be shorter, clearer, and prone to one less boundary condition as: 
    foreach my $month (1 ... 12)
{

    [download]
    The biggest issue you have is what appears to be a complete reliance on package globals. If you forget to reset one to zero at the start of the sub, you'll have values bleading over across invocations -- exactly what you describe. Change all of these variables into lexicals within your sub and I suspect your error will disappear.
[reply]
[d/l]
[select]
Re: Logic problem on loops
by gav^ (Curate) on Apr 21, 2002 at 21:41 UTC
    Some pointers:
    • Use better variable names, things like $trf, $trft etc are too easy to confuse
    • Use different quotes (look up the qq// operator) so you don't have to escape quotes all the time
    • Better yet, use something like HTML::Template (or any other templating module) so you don't have to embed HTML in your scripts
    • If something is a number, treat it like a number. Use $months == 2 instead of $months eq "2"
    • Make sure the plurality of the variable name is right, eg $month instead of $months
    • Use my and stop using global variables
    Replace code like: 
        while (@dataf = $sthf->fetchrow_array())
    {
        $filiais[$xf] = $dataf[0];
        $xf++;
    }

    [download]
    with calls that use selectall_arrayref. Use bind so you can write: 
    $sth->execute;
$sth->bind_columns(\my $x, \my $y);
while ($sth->fetch) {
   # use $x and #y
}

    [download]
    Use placeholders to save issues with quoting: 
    my $sth = $dbh->prepare('select w from x where y = ? and z = ?');
$sth->execute($y, $z);

    [download]
    You've also got quite a bit of code that looks repeated. Maybe you can refactor it out into some loops or subroutine calls. You should try to make functions shorter in length so that it's easier to comprehend and you don't have to keep scrolling up and down so much.

    That's all for now...

    gav^

[reply]
[d/l]
[select]
Re: Logic problem on loops
by dws (Chancellor) on Apr 21, 2002 at 20:40 UTC
    Using one column for each month makes it rather hard to get the database to some of this work for you. If instead of the "f1".."f12", etc., columns you had a "month" column and single columns for "f", "m", "rf", and "rm", you could do the month selection and trimester rollups via a query. A bit of reading on relational database "normal forms" and their benefits might pay off for you.

    I'm suspicious of the DISTINCT in that second SELECT. Are you really sure you're gettin back the data you want?

    Without knowing more about the data, it's hard to know whether you have a problem tripping the month tests multiple times. I suggest adding 

        if ($month eq "1") #January, obviously
    {
>>>     $message .= "<!-- January -->\n";

    [download]
    and so on for the other months. This will tell you whether you're accidentally tripping those tests multiple times, and clobbering counters.

[reply]
[d/l]
Re: Logic problem on loops
by graff (Chancellor) on Apr 21, 2002 at 21:39 UTC
    Why is there nothing in your second query ("SELECT DISTINCT " .$fa...) to specify which month you're working on? Do you get the exact same too-big numbers for all trimesters? I would expect this to happen, because it looks like you are doing 

       while (@rown = $sthn->fetchrow_array())

    [download]

    over the same set of rows in each of the twelve iterations through the months.

    So, does the DB schema provide any information to identify which month applies to each of the various rows containing "first" and "second" "predicted" and "realized" values? If so, work that into the query. If not, you can't do the trimester tabulation at all, no matter how good your Perl code is.

    If/when you solve that problem, then think of the tabulation this way (it will make the code shorter and easier): 

    foreach $filiais (@filiais){
  # print an html header (you don't need $cnt2 for this)
  $tf = $trf = $tm = $trm = 0; # set accumulators to 0
  foreach $month (1 .. 12) {
    # query DB for rows of 4 values _for_this_month_
    while (@rown = $sthn->fetchrow_array()) {
       ($fa,$rfa,$ma,$rma) = @rown;
       $tf += $fa;  # and likewise for the other three accumulators
    }
    if ( $month % 3 == 0 ) { # true for months 3,6,9,12
      ListTrimestre( $tf,$trf,$tm,$trm,$month/3 );
      $tf = $trf = $tm = $trm = 0; # reset accumulators to 0
    }
  }
}

    [download]
[reply]
[d/l]
[select]
      OOOPS! I'm sorry -- I missed how you were working the month number into the query. But "dws" is right about the structure of the table; I was assuming that "month" information would naturally be in a column of its own.

      In any case, I'd still recommend trying the approach in my ealier reply -- a single block that does the same thing for every month, with a little sub-block that does the special thing at the end of each trimester.

      BTW, is there any chance that data from multiple years are getting mixed into your tables, or are you certain that the table starts out empty every January?

[reply]

Back to Seekers of Perl Wisdom