Help paring down massive subroutine

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

Perl Monks-- I have recently written my first moderately sized web app in Perl. Everything works well, but I have one subroutine that is massive. I'm sure that there is a better way to do things than how I've proceeded. I would really appreciate any help in paring down this routine. --Tim Howell

sub save_timecard{

    my $self = shift;

    my $template = $self->load_tmpl('timecard.htm');
    
    my $db = $self->param('my_db');

    my $cgi_query = $self->query();

    #Setup session using CGI::Session
    my $sid = $cgi_query->param('sid') || undef;
    my $session = new CGI::Session::File($sid, { Directory => 'C:\inet
+pub\wwwroot\Time\stmp' });
    my $employeeID = $session->param('EmployeeID');
    my $is_supervisor = $session->param('Supervisor');

    my $cardID = $cgi_query->param('cardID');
    my $change_carddate = $cgi_query->param('carddate');
    my $cardEmployeeID = $cgi_query->param('empID');

    #This is one of the few run modes that requires transactions to be
+ used in the DBMS
    #Because timecards save information in several tables (Employees, 
+TimeCards, Remarks), in several distinct steps,
    #we want to make sure that everything goes or nothing goes.
    #The BEGIN happens before any of the statements.  A ROLLBACK shoul
+d be executed before any errors are thrown.
    #COMMIT takes place just before returning the filled-in HTML::Temp
+late to the user at the end of the run mode.

    my $sql_transaction_statement = "BEGIN;";
    my $transaction_sth = $db->prepare($sql_transaction_statement);
    $transaction_sth->execute();

    my @day_prefixes = ('m', 't', 'w', 'r', 'f', 's', 'su');

    my $day_counter = 0;
    my $execute_query = 0;
    my $top_in_rounding = $session->param("TopInRounding");
    my $bottom_in_rounding = $session->param("BottomInRounding");
    my $top_out_rounding = $session->param("TopOutRounding");
    my $bottom_out_rounding = $session->param("BottomOutRounding");
    foreach $day_prefix (@day_prefixes){
        $execute_query = 1;
        my $MornIn = fix_time_format($cgi_query->param($day_prefix . '
+MornIn') || 'NULL');
        $MornIn = round_time($MornIn, $top_in_rounding, $bottom_in_rou
+nding);
        my $MornOut = fix_time_format($cgi_query->param($day_prefix . 
+'MornOut') || 'NULL');
        $MornOut = round_time($MornOut, $top_out_rounding, $bottom_out
+_rounding);
        if (($MornIn ne 'NULL') && ($MornOut ne 'NULL') && ($MornIn > 
+$MornOut) && ($MornOut > 0)){
            $MornOut = add_twelve($MornOut);
        }
        my $AftIn = fix_time_format($cgi_query->param($day_prefix . 'A
+ftIn') || 'NULL');
        $AftIn = round_time($AftIn, $top_in_rounding, $bottom_in_round
+ing);
        my $AftOut = fix_time_format($cgi_query->param($day_prefix . '
+AftOut') || 'NULL');
        $AftOut = round_time($AftOut, $top_out_rounding, $bottom_out_r
+ounding);
        if (($AftIn ne 'NULL') && ($AftOut ne 'NULL') && ($AftIn > $Af
+tOut) && ($AftOut > 0)){
            $AftOut = add_twelve($AftOut);
        }
        my $NightIn = fix_time_format($cgi_query->param($day_prefix . 
+'NightIn') || 'NULL');
        $NightIn = round_time($NightIn, $top_in_rounding, $bottom_in_r
+ounding);
        my $NightOut = fix_time_format($cgi_query->param($day_prefix .
+ 'NightOut') || 'NULL');
        $NightOut = round_time($NightOut, $top_out_rounding, $bottom_o
+ut_rounding);
        if (($NightIn ne 'NULL') && ($NightOut ne 'NULL') && ($NightIn
+ > $NightOut) && ($NightOut > 0)){
            $NightOut = add_twelve($NightOut);
        }

        my $sick = $cgi_query->param($day_prefix . 'Sick') || 'NULL';
        $sick = ($sick * 3600) unless ($sick eq 'NULL');
        my $vacation = $cgi_query->param($day_prefix . 'Vacation') || 
+'NULL';
        $vacation = ($vacation * 3600) unless ($vacation eq 'NULL');
        
        #Build SQL and send it to DB
        my $sql_statement = "UPDATE TimeCards SET\n";
        $sql_statement .= ", MornIn=$MornIn\n" if $MornIn;
        $sql_statement .= ", MornOut=$MornOut\n" if $MornOut;
        $sql_statement .= ", AftIn=$AftIn\n" if $AftIn;
        $sql_statement .= ", AftOut=$AftOut\n" if $AftOut;
        $sql_statement .= ", NightIn=$NightIn\n" if $NightIn;
        $sql_statement .= ", NightOut=$NightOut\n" if $NightOut;
        $sql_statement .= ", Sick=$sick\n" if $sick;
        $sql_statement .= ", Vacation=$vacation\n" if $vacation;
        $execute_query = 1 if ($sql_statement =~ s/, //);    #Strip le
+ading ", " from query.  
        #Also set flag to run query if a leading , was stripped (meani
+ng that the query contains meaningful SQL)
        $sql_statement .= "WHERE CardDate='$change_carddate'\n";
        $sql_statement .= "AND EmployeeID='$cardEmployeeID'\n";
        $sql_statement .= "AND DayNumber=$day_counter;";

        #return $sql_statement;

        if ($execute_query){
        #return $sql_statement;
            my $sth = $db->prepare($sql_statement);
            $sth->execute();
        }
        $day_counter++;
    }

    $day_counter = 0;
    $execute_query = 0;
    my $consecutive_day_counter = 0;
    my $regular_time_total = 0;    #If this number exceeds ($week_over
+time hours * 3600 seconds/hour) then begin paying out overtime
    #Preferences that affect time calculation:
    my $week_overtime = $session->param("WeekOvertime");
    my $day_overtime = $session->param("DayOvertime");
    my $day_doubletime = $session->param("DayDoubletime");
    my $seventh_day_overtime = $session->param("SeventhDayOvertime");
    foreach $day_prefix (@day_prefixes){

        #Rather than have the DBMS do all the math (with ridiculously 
+complicated SQL),
        #the DBMS will only do addition to get all of the seconds and 
+the script will figure out what to
        #insert in each column (Regular, Overtime, Doubletime)

        $sql_query = "SELECT ABS(IFNULL((TIME_TO_SEC(MornOut) - TIME_T
+O_SEC(MornIn)), 0)\n";
        $sql_query .= "+ IFNULL((TIME_TO_SEC(AftOut) - TIME_TO_SEC(Aft
+In)), 0)\n";
        $sql_query .= "+ IFNULL((TIME_TO_SEC(NightOut) - TIME_TO_SEC(N
+ightIn)), 0))\n";
        $sql_query .= "AS TotalSeconds\n";
        $sql_query .= "FROM TimeCards WHERE CardDate='$change_carddate
+'\n";
        $sql_query .= "AND PayPeriodID=0\n";
        $sql_query .= "AND EmployeeID=$cardEmployeeID\n";
        $sql_query .= "AND DayNumber=$day_counter;\n";

        my $sth = $db->prepare($sql_query);
        $sth->execute();
        my $overtime = 'NULL';
        my $doubletime = 'NULL';
        my $data_ref;
        if ($data_ref = $sth->fetchrow_hashref()){
            #Hours were entered for this day, so increase $consecutive
+_day_counter
            my %data = %$data_ref;
            $regular = $data{"TotalSeconds"};
            if ($regular > 0){
                $consecutive_day_counter++;
            }else{
                $consecutive_day_counter = 0;
            }
            if (($consecutive_day_counter == 7) && $seventh_day_overti
+me){
                $overtime = $regular;
                $overtime = 'NULL' if (!($overtime > 0));
                $regular = 'NULL';
                if ($overtime > ($day_overtime * 3600)){
                    $overtime = ($day_overtime * 3600);
                    $doubletime = ($data{"TotalSeconds"} - ($day_overt
+ime * 3600));
                }    
            }elsif ($regular > ($day_overtime * 3600)){
                $regular = ($day_overtime * 3600);
                $overtime = ($data{"TotalSeconds"} - ($day_overtime * 
+3600));
                if ($overtime > (($day_doubletime - $day_overtime) * 3
+600)){
                    $overtime = (($day_doubletime - $day_overtime) * 3
+600);
                    $doubletime = ($data{"TotalSeconds"} - ($day_doubl
+etime * 3600));
                }
            }
        }else{
            $consecutive_day_counter = 0;
            $regular = 'NULL';
        }
        $regular = 'NULL' if ($regular == 0);
        
        $regular_time_total = $regular_time_total + $regular unless ($
+regular eq 'NULL');
        if (($regular_time_total > ($week_overtime * 3600)) && ($regul
+ar > 0)){
            $overtime = $overtime + $regular;
            $regular = 'NULL';
        }
    
        $sql_statement = "UPDATE TimeCards SET\n";
        $sql_statement .= "Regular=$regular,\n";
        $sql_statement .= "Overtime=$overtime,\n";
        $sql_statement .= "Doubletime=$doubletime\n";
        $sql_statement .= "WHERE CardDate='$change_carddate'\n";
        $sql_statement .= "AND EmployeeID='$cardEmployeeID'\n";
        $sql_statement .= "AND DayNumber=$day_counter;";

        #return $sql_statement;

        $sth = $db->prepare($sql_statement);
        $sth->execute();
        $day_counter++;
    }

    #Insert remarks into Remarks table
    my $remarks = $cgi_query->param('remarks');
    if ($remarks){
        my $sql_statement3;
        my $sql_query2 = "SELECT Remarks FROM Remarks WHERE EmployeeID
+='$cardEmployeeID'\n";
        $sql_query2 .= "AND CardDate='$change_carddate';";

        my $sth2 = $db->prepare($sql_query2);
        $sth2->execute();

        $remarks = $db->quote($remarks);    #Escape necessary characte
+rs before passing into DBMS

        if ($sth2->fetchrow_hashref()){
            $sql_statement3 = "UPDATE Remarks SET\n";
            $sql_statement3 .= "Remarks=$remarks\n";
            $sql_statement3 .= "WHERE EmployeeID='$cardEmployeeID'\n";
            $sql_statement3 .= "AND CardDate='$change_carddate';";
        }else{
            $sql_statement3 = "INSERT INTO Remarks (EmployeeID, CardDa
+te, Remarks)\n";
            $sql_statement3 .= "VALUES ('$cardEmployeeID', '$change_ca
+rddate', $remarks);";
        }

        my $sth3 = $db->prepare($sql_statement3);
        $sth3->execute();
    }

    #Update vacation and sick leave balances
    my $sql_query4 = "SELECT IFNULL(SUM(Vacation), 0) AS UsedVacation,
+ IFNULL(SUM(Sick), 0) AS UsedSick\n";
    $sql_query4 .= "FROM TimeCards\n";
    $sql_query4 .= "WHERE EmployeeID='$cardEmployeeID';";

    my $sth4 = $db->prepare($sql_query4);
    $sth4->execute();

    my $data_ref = $sth4->fetchrow_hashref();
    my %data = %$data_ref;

    my $used_vacation = $data{"UsedVacation"};
    my $used_sick = $data{"UsedSick"};

    my $sql_query5 = "SELECT (AnnualVacation * 3600) AS PreVacationBal
+ance,\n";
    $sql_query5 .= "(AnnualSick * 3600) AS PreSickLeaveBalance\n";
    $sql_query5 .= "FROM Employees\n";
    $sql_query5 .= "WHERE EmployeeID='$cardEmployeeID';";

    my $sth5 = $db->prepare($sql_query5);
    $sth5->execute();

    if (!($data_ref = $sth5->fetchrow_hashref())){
        #Error.  Rollback DB:
        $sql_transaction_statement = "ROLLBACK;";
        $transaction_sth = $db->prepare($sql_transaction_statement);
        $transaction_sth->execute();
        #And send error message:
        my $output = "Error calculating remaining vacation and sick ti
+me.  Please see administrator<BR>\n";
        return $output;
    }
    %data2 = %$data_ref;
    my $output;
    if (($data2{"PreVacationBalance"} - $used_vacation) < 0){
        $output .= "Not enough vacation time remaining to fulfill this
+ request.  Please see administrator<BR>\n";
    }
    if (($data2{"PreSickLeaveBalance"} - $used_sick) < 0){
        $output .= "Not enough sick leave remaining to fulfill this re
+quest.  Please see administrator<BR>\n";
    }
    if ($output){
        #Error.  Rollback DB.
        $sql_transaction_statement = "ROLLBACK;";
        $transaction_sth = $db->prepare($sql_transaction_statement);
        $transaction_sth->execute();
        #And send error message
        return $self->display_error($output);
    }

    my $sql_statement6 = "UPDATE Employees SET\n";
    $sql_statement6 .= "VacationBalance=($data2{'PreVacationBalance'} 
+- $used_vacation),\n";
    $sql_statement6 .= "SickLeaveBalance=($data2{'PreSickLeaveBalance'
+} - $used_sick)\n";
    $sql_statement6 .= "WHERE EmployeeID='$cardEmployeeID';";
    
    my $sth6 = $db->prepare($sql_statement6);
    $sth6->execute;

    #Everything looks good.  Commit changes to DB
    $sql_transaction_statement = "COMMIT;";
    $transaction_sth = $db->prepare($sql_transaction_statement);
    $transaction_sth->execute();

    #Return user to updated timecard
    $self->param('carddate' => $change_carddate);
    $self->param('empID' => $cardEmployeeID);
    return $self->display_timecard();

}    #End of sub save_timecard
[download]

Comment on Help paring down massive subroutine Download Code

Replies are listed 'Best First'.
Re: Help paring down massive subroutine by John M. Dlugosz (Monsignor) on Nov 14, 2002 at 20:08 UTC
To simply chop it up, find natural boundaries where the set of which variables are needed changes. A large number of concecutive lines that uses only a small number of the total number of local variables could be pulled into a subroutine, passing it only those. If there are no such divisions, make an object to hold the stuff as instance data, and then you can divide anywhere. Divide it into separate members based on logically complete steps.	[reply]
Re: Help paring down massive subroutine by jdporter (Paladin) on Nov 14, 2002 at 20:22 UTC
In addition to what John M. Dlugosz said, I think where you have put comments is probably a good clue as to where the logical divisions of the code are. jdporter ...porque es dificil estar guapo y blanco.	[reply]
Re: Help paring down massive subroutine by janjan (Beadle) on Nov 14, 2002 at 20:36 UTC
In addition to what the other two have suggested, one specific thing you can do is try removing each database query to its own subroutine. Since each statement handle is its own logical unit, and returns a different thing, it would be easy to write them as individual subroutines that accept parameters to look for (like $MornIn and $MornOut), and return either success for an UPDATE or specific values for a SELECT.	[reply]
Re: Help paring down massive subroutine by graff (Chancellor) on Nov 15, 2002 at 04:48 UTC
In terms of actually reducing the overall bulk, the first loop over @day_prefixes seems to be quite long with a lot of repetitive stuff. Given how nicely you have kept variable names, param names and DB field names consistent, I'd wager that you'll find it fairly easy to reduce quite a bit of that code into a nested loop over these names. Use a hash keyed by field name to store values that should be sent to the DB, rather than using separate scalars named for each field -- e.g.: `$sql_statement = "update timecard set " foreach $fname ( keys %fieldhash ) { $sql_statement .= "$fname = $fieldhash{$fname}," if ( $fieldhash{$ +fname} ); } $sql_statement =~ s/,$/ WHERE /; $sql_statement .= "blah = blah etc...";` [download] You might even find a way to extend the use of that hash so that the field names involved appear only once or twice in the whole subroutine, rather than several times (and likewise for other parameter/field sets). Personally, I always find it a nice feature when I can reduce the number of times that I write a given set of names in a script.	[reply] [d/l]