in reply to Re^3: Cleaning up Code
in thread Cleaning up Code

Thank you again for your response. I was hoping I could provide this snippet and take what I learn from it and clean up my entire script. However, it appears as if I may need to show you my entire script to ensure I don't shoot myself in the foot later. I appreciate the time you've already spent on this with me but would you be willing, if I show you my entire script, to provide me with some guidance/suggestions to get my script cleaned up?

Replies are listed 'Best First'.
Re^5: Cleaning up Code
by ikegami (Patriarch) on Dec 10, 2008 at 06:17 UTC
    Sure, I can have a look at it.
[reply]
      What is the best way to get this to you, I'd much rather send it to you than post it on the site as it is a bunch of code. --update-- Actually, I'll be honest.. I didn't want to post it all in fear of being "flamed" of bad usage/practice, but I'll face the humility and post all the code here. Here it is: -- update -- I have updated this code, and it appears as if my only issues are in the code you suggested, any thoughts? 
      #!/usr/bin/perl

use DBI();
use PDF::Create;
use Digest::SHA qw(sha1 sha1_hex sha1_base64);
use MIME::Lite;
use HTML::Template;
use strict;
use warnings;
use CGI qw(:standard);

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

my $template = HTML::Template->new(filename => 'incident.tmpl.html');
my $insert = qq{INSERT INTO incident_report (incident_type, incident_d
+esc, security_impact, reported_by, date_reported, offender_name, offe
+nder_userid, offender_machineid, action_taken, action_date, time_spen
+t, incident_sha) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
} or dienice(dbh->errstr);
my $query = qq{SELECT incident_sha FROM incident_report WHERE incident
+_sha = ?} or dienice(dbh->errstr);

#Sub-routines
sub create_pdf {
my $time = localtime();

my $pdf = new PDF::Create('filename'     => "../pdfs/ir_$_[11].pdf",
                       'Version'      => 1.2,
                       'PageMode'     => 'UseNone',
                       'Author'       => 'author',
                       'Title'        => "ir_$_[11].pdf",
                       'CreationDate' => [ localtime ],
                        );

my $root = $pdf->new_page('MediaBox' => $pdf->get_page_size('A4'));
my $page = $root->new_page;
my $f1 = $pdf->font('Subtype'  => 'Type1',
                 'Encoding' => 'WinAnsiEncoding',
                 'BaseFont' => 'Helvetica');
my $f2 = $pdf->font('Subtype'  => 'Type1',
                 'Encoding' => 'WinAnsiEncoding',
                 'BaseFont' => 'Helvetica-Bold');
my $toc = $pdf->new_outline('Title' => 'Incident Report',
                         'Destination' => $page);
$page->stringc($f2, 30, 306, 800, "Incident Report");
$page->stringc($f1, 10, 306, 782, "created : $time");
$page->stringc($f2, 15, 306, 750, "FORM FOR RECORDING SECURITY INCIDEN
+T RESPONSES");
$page->stringc($f2, 10, 306, 716, "Incident # (online only) = $_[11]")
+;
$page->stringc($f2, 15, 125, 675, "Incident Type");
$page->stringc($f2, 15, 125, 625, "Security Impact");
$page->stringc($f2, 15, 125, 575, "Incident Description");
$page->stringc($f2, 15, 125, 525, "Reported By");
$page->stringc($f2, 15, 125, 475, "Date Reported");
$page->stringc($f2, 15, 125, 425, "Offender's Full Name");
$page->stringc($f2, 15, 125, 375, "Offender's UserID");
$page->stringc($f2, 15, 125, 325, "Offender's MachineID");
$page->stringc($f2, 15, 125, 275, "Action Taken");
$page->stringc($f2, 15, 125, 225, "Date Action Taken");
$page->stringc($f2, 15, 125, 175, "Time Spent");
$page->stringl($f2, 15, 36, 92, "Closure Authorized by:");
$page->stringl($f2, 15, 410, 92, "Date:");
$page->stringl($f1, 15, 220, 675, "$_[0]");
$page->stringl($f1, 9, 220, 575, "$_[1]");
$page->stringl($f1, 15, 220, 625, "$_[2]");
$page->stringl($f1, 15, 220, 525, "$_[3]");
$page->stringl($f1, 15, 220, 475, "$_[4]");
$page->stringl($f1, 15, 220, 425, "$_[5]");
$page->stringl($f1, 15, 220, 375, "$_[6]");
$page->stringl($f1, 15, 220, 325, "$_[7]");
$page->stringl($f1, 9, 220, 275, "$_[8]");
$page->stringl($f1, 15, 220, 225, "$_[9]");
$page->stringl($f1, 15, 220, 175, "$_[10]");
$page->newpath;
$page->rectangle(36,740,530,30);
$page->rectangle(36,660,180,40);
$page->rectangle(216,660,350,40);
$page->rectangle(36,610,180,40);
$page->rectangle(216,610,350,40);
$page->rectangle(36,560,180,40);
$page->rectangle(216,560,350,40);
$page->rectangle(36,510,180,40);
$page->rectangle(216,510,350,40);
$page->rectangle(36,460,180,40);
$page->rectangle(216,460,350,40);
$page->rectangle(36,410,180,40);
$page->rectangle(216,410,350,40);
$page->rectangle(36,360,180,40);
$page->rectangle(216,360,350,40);
$page->rectangle(36,310,180,40);
$page->rectangle(216,310,350,40);
$page->rectangle(36,260,180,40);
$page->rectangle(216,260,350,40);
$page->rectangle(36,210,180,40);
$page->rectangle(216,210,350,40);
$page->rectangle(36,160,180,40);
$page->rectangle(216,160,350,40);
$page->rectangle(75,710,455,20);
$page->line(36,90,530,90);
$page->closestroke;

$pdf->close;
}

sub dienice {
        $template->param(DBERROR => @_);
        print $template->output;
        exit;
}

#Parse Data
my %FORM;
foreach my $field (param()) {
    $FORM{$field} = param($field);
}


#Error Descriptions
my $dberror = "";
my $error = "";
my $itype = "*Please provide a valid incident type.<br>";
my $idesc = "*Please provide an incident description.<br>";
my $impat = "*Please provide a valid security impact.<br>";
my $repby = "*Please provide the full name of the person who reported 
+this incident.<br>";
my $date0 = "*Please provide the date this incident was reported in th
+e proper format.<br>";
my $atake = "*Please provide the action taken to remedy the incident.<
+br>";
my $adate = "*Please provide the date the remedy was applied in the pr
+oper format.<br>";
my $oname = "*Please provide the full name of the offender.<br>";
my $ousid = "*Please provide the offender's usersid.<br>";
my $omaid = "*Please provide the offender's machineid.<br>";
my $spent = "*Please provide the amount of time you spent on this offe
+nder in the proper format<br>";
my $ndata = "*You have failed to fill in all the required fields, plea
+se try again.<br>";

#HTML::Template VAR's
$template->param(ITYPE => $FORM{'incident_type'});
$template->param(SIMPACT => $FORM{'security_impact'});
$template->param(IDESC => $FORM{'incident_description'});
$template->param(REPORTED => $FORM{'reported_by'});
$template->param(DATE0 => $FORM{'date0'});
$template->param(ONAME1 => $FORM{'offender_name1'});
$template->param(ONAME2 => $FORM{'offender_name2'});
$template->param(ONAME3 => $FORM{'offender_name3'});
$template->param(ONAME4 => $FORM{'offender_name4'});
$template->param(ONAME5 => $FORM{'offender_name5'});
$template->param(OUSID1 => $FORM{'offender_userid1'});
$template->param(OUSID2 => $FORM{'offender_userid2'});
$template->param(OUSID3 => $FORM{'offender_userid3'});
$template->param(OUSID4 => $FORM{'offender_userid4'});
$template->param(OUSID5 => $FORM{'offender_userid5'});
$template->param(ATAKEN1 => $FORM{'action_taken1'});
$template->param(ATAKEN2 => $FORM{'action_taken2'});
$template->param(ATAKEN3 => $FORM{'action_taken3'});
$template->param(ATAKEN4 => $FORM{'action_taken4'});
$template->param(ATAKEN5 => $FORM{'action_taken5'});
$template->param(DATE1 => $FORM{'date1'});
$template->param(DATE2 => $FORM{'date2'});
$template->param(DATE3 => $FORM{'date3'});
$template->param(DATE4 => $FORM{'date4'});
$template->param(DATE5 => $FORM{'date5'});
$template->param(TSPENT1 => $FORM{'time_spent1'});
$template->param(TSPENT2 => $FORM{'time_spent2'});
$template->param(TSPENT3 => $FORM{'time_spent3'});
$template->param(TSPENT4 => $FORM{'time_spent4'});
$template->param(TSPENT5 => $FORM{'time_spent5'});
$template->param(OMACH1 => $FORM{'offender_machineid1'});
$template->param(OMACH2 => $FORM{'offender_machineid2'});
$template->param(OMACH3 => $FORM{'offender_machineid3'});
$template->param(OMACH4 => $FORM{'offender_machineid4'});
$template->param(OMACH5 => $FORM{'offender_machineid5'});

#Form Validation
if ($FORM{'incident_type'} =~ /Virus|Spyware|Theft|Destruction|Disclos
+ure|Misuse|Hacking|Error|Failure/)
{       if ($FORM{'security_impact'} =~ /Terminal|Devastating|Critical
+|Controllable|Irritating/)
        {       if ($FORM{'incident_description'} =~ /\w+/)
                {       if ($FORM{'reported_by'} =~ /\w+\s\w+/)
                        {       if ($FORM{'date0'} =~ /\d{4,4}\/\d{2,2
+}\/\d{2,2}/)
                                {       my @required = ($FORM{'inciden
+t_type'}, $FORM{'incident_description'}, $FORM{'security_impact'}, $F
+ORM{'reported_by'}, $FORM{'date0'})}
                                else {$error .= $date0}
                        }else {$error .= $repby}
                }else {$error .= $idesc}
        }else {$error .= $impat}
}else {$error .= $itype}
;

if ($FORM{'offender_name1'} eq "" && $FORM{'offender_name2'} eq "" && 
+$FORM{'offender_name3'} eq "" && $FORM{'offender_name4'} eq "" && $FO
+RM{'offender_name5'} eq "")
{$error .= $oname}

if ($FORM{'offender_name1'} =~ /\w+\s\w+/)
{       if ($FORM{'offender_userid1'} =~ /\w+/)
        {       if ($FORM{'action_taken1'} =~ /\w+/)
                {       if ($FORM{'date1'} =~ /\d{4,4}\/\d{2,2}\/\d{2,
+2}/)
                        {       if ($FORM{'time_spent1'} =~ /\d{2,2}\:
+\d{2,2}/)
                                {       if ($FORM{'offender_machineid1
+'} !~ /\w+/) {
                                                {       $FORM{'offende
+r_machineid1'} = "n/a";
                                                        my @row1 = ($F
+ORM{'offender_name1'}, $FORM{'offender_userid1'}, $FORM{'offender_mac
+hineid1'}, $FORM{'action_taken1'}, $FORM{'date1'}, $FORM{'time_spent1
+'})};
                                                } elsif ($FORM{'offend
+er_machineid1'} =~ /\w+/) {
                                                        my @row1 = ($F
+ORM{'offender_name1'}, $FORM{'offender_userid1'}, $FORM{'offender_mac
+hineid1'}, $FORM{'action_taken1'}, $FORM{'date1'}, $FORM{'time_spent1
+'})};

                                } else {$error .= $spent}
                        } else {$error .= $adate}
                } else {$error .= $atake}
        } else {$error .= $ousid}
}
;

if ($FORM{'offender_name2'} =~ /\w+\s\w+/)
{       if ($FORM{'offender_userid2'} =~ /\w+/)
        {       if ($FORM{'action_taken2'} =~ /\w+/)
                {       if ($FORM{'date2'} =~ /\d{4,4}\/\d{2,2}\/\d{2,
+2}/)
                        {       if ($FORM{'time_spent2'} =~ /\d{2,2}\:
+\d{2,2}/)
                                {       if ($FORM{'offender_machineid2
+'} !~ /\w+/) {
                                                {       $FORM{'offende
+r_machineid2'} = "n/a";
                                                        my @row2 = ($F
+ORM{'offender_name2'}, $FORM{'offender_userid2'}, $FORM{'offender_mac
+hineid2'}, $FORM{'action_taken2'}, $FORM{'date2'}, $FORM{'time_spent2
+'})};
                                                } elsif ($FORM{'offend
+er_machineid2'} =~ /\w+/) {
                                                        my @row2 = ($F
+ORM{'offender_name2'}, $FORM{'offender_userid2'}, $FORM{'offender_mac
+hineid2'}, $FORM{'action_taken2'}, $FORM{'date2'}, $FORM{'time_spent2
+'})};

                                } else {$error .= $spent}
                        } else {$error .= $adate}
                } else {$error .= $atake}
        } else {$error .= $ousid}
}
;

if ($FORM{'offender_name3'} =~ /\w+\s\w+/)
{       if ($FORM{'offender_userid3'} =~ /\w+/)
        {       if ($FORM{'action_taken3'} =~ /\w+/)
                {       if ($FORM{'date3'} =~ /\d{4,4}\/\d{2,2}\/\d{2,
+2}/)
                        {       if ($FORM{'time_spent3'} =~ /\d{2,2}\:
+\d{2,2}/)
                                {       if ($FORM{'offender_machineid3
+'} !~ /\w+/) {
                                                {       $FORM{'offende
+r_machineid3'} = "n/a";
                                                        my @row3 = ($F
+ORM{'offender_name3'}, $FORM{'offender_userid3'}, $FORM{'offender_mac
+hineid3'}, $FORM{'action_taken3'}, $FORM{'date3'}, $FORM{'time_spent3
+'})};
                                                } elsif ($FORM{'offend
+er_machineid3'} =~ /\w+/) {
                                                        my @row3 = ($F
+ORM{'offender_name3'}, $FORM{'offender_userid3'}, $FORM{'offender_mac
+hineid3'}, $FORM{'action_taken3'}, $FORM{'date3'}, $FORM{'time_spent3
+'})};

                                } else {$error .= $spent}
                        } else {$error .= $adate}
                } else {$error .= $atake}
        } else {$error .= $ousid}
}
;

if ($FORM{'offender_name4'} =~ /\w+\s\w+/)
{       if ($FORM{'offender_userid4'} =~ /\w+/)
        {       if ($FORM{'action_taken4'} =~ /\w+/)
                {       if ($FORM{'date4'} =~ /\d{4,4}\/\d{2,2}\/\d{2,
+2}/)
                        {       if ($FORM{'time_spent4'} =~ /\d{2,2}\:
+\d{2,2}/)
                                {       if ($FORM{'offender_machineid4
+'} !~ /\w+/) {
                                                {       $FORM{'offende
+r_machineid4'} = "n/a";
                                                        my @row4 = ($F
+ORM{'offender_name4'}, $FORM{'offender_userid4'}, $FORM{'offender_mac
+hineid4'}, $FORM{'action_taken4'}, $FORM{'date4'}, $FORM{'time_spent4
+'})};
                                                } elsif ($FORM{'offend
+er_machineid4'} =~ /\w+/) {
                                                        my @row4 = ($F
+ORM{'offender_name4'}, $FORM{'offender_userid4'}, $FORM{'offender_mac
+hineid4'}, $FORM{'action_taken4'}, $FORM{'date4'}, $FORM{'time_spent4
+'})};

                                } else {$error .= $spent}
                        } else {$error .= $adate}
                } else {$error .= $atake}
        } else {$error .= $ousid}
}
;

if ($FORM{'offender_name5'} =~ /\w+\s\w+/)
{       if ($FORM{'offender_userid5'} =~ /\w+/)
        {       if ($FORM{'action_taken5'} =~ /\w+/)
                {       if ($FORM{'date5'} =~ /\d{4,4}\/\d{2,2}\/\d{2,
+2}/)
                        {       if ($FORM{'time_spent5'} =~ /\d{2,2}\:
+\d{2,2}/)
                                {       if ($FORM{'offender_machineid5
+'} !~ /\w+/) {
                                                {       $FORM{'offende
+r_machineid5'} = "n/a";
                                                        my @row5 = ($F
+ORM{'offender_name5'}, $FORM{'offender_userid5'}, $FORM{'offender_mac
+hineid5'}, $FORM{'action_taken5'}, $FORM{'date5'}, $FORM{'time_spent5
+'})};
                                                } elsif ($FORM{'offend
+er_machineid5'} =~ /\w+/) {
                                                        my @row5 = ($F
+ORM{'offender_name5'}, $FORM{'offender_userid5'}, $FORM{'offender_mac
+hineid5'}, $FORM{'action_taken5'}, $FORM{'date5'}, $FORM{'time_spent5
+'})};

                                } else {$error .= $spent}
                        } else {$error .= $adate}
                } else {$error .= $atake}
        } else {$error .= $ousid}
}
;

#HTML Feedback
if ($error ne "")
{
$template->param(ERROR => $error);
print $template->output;
}
else
{

#Email Digital Copy to ISO
my $msg = MIME::Lite->new(
       From    => 'pitcher',
       To      => 'catcher',
#       Cc      => '',
       Subject => 'Incident Reports',
       Type    => 'multipart/mixed'
);



#Define Database
my $dbh = DBI->connect("DBI:mysql:database=;host=",
                    "","") or &dienice($DBI::errstr);

#$sth = $dbh->prepare($insert);

#Insert Data
for my $row ( 1 .. $#rows ) {
   if (@{ $rows[$row] } == "6") {
      my @entry = (@required , @row);
      my $digest = sha1_hex(@entry);
      my $sth = $dbh->prepare($query);
      $sth->execute($digest);
      my $data = $sth->fetchrow_array();
      if ($data ne $digest) {
         push(@entry, $digest);
         my $sth = $dbh->prepare($insert);
         $sth->execute(@entry);
         create_pdf(@entry);
         $msg->attach(
            Type     => 'image/gif',
            Path     => "../pdfs/ir_$digest.pdf",
            Filename => "ir_$digest.pdf",
            Disposition => 'attachment')
        }
   } else {
        my $dberror .= "The following row you submitted: <br> @entry <
+br>already exsits in the database and will note be re-submitted. <br>
+"
}
};

$dbh->disconnect;

$msg->send;

if ($dberror ne "") {
        dienice($dberror)
} else {

print <<HTML_SUCCESS

<html><head><title>Confirmation</title>
</head><body>
<h1 align="center">Confirmation</h1>
<p style="text-align: center;">Congratulations you have submitted your
+ information successfully.  Below is the information that has been su
+bmitted.  If the infomation below is not accurate and/or you just rea
+lized the data has an error please notify me.<p>
<TABLE border="1" align="center" summary="">
<CAPTION><EM><BIG><STRONG>Submitted Information</STRONG></BIG></EM></C
+APTION>
<tr>
<th>Incident Type</th><th></b>Brief Description</th><th></b>Security I
+mpact</th><th></b>Reported By</th><th></b>Date Reported</th><th></b>O
+ffender's Full Name</th><th></b>Offender's UserID</th><th></b>Offende
+r's MachineID</th><th></b>Action Taken</th><th></b>Date Action Taken<
+/th><th></b>Time Spent</th><th></b>Incident Number
</tr>
<tr>
<td>$entry1[0]</td><td>$entry1[1]</td><td>$entry1[2]</td><td>$entry1[3
+]</td><td>$entry1[4]</td><td>$entry1[5]</td><td>$entry1[6]</td><td>$e
+ntry1[7]</td><td>$entry1[8]</td><td>$entry1[9]</td><td>$entry1[10]</t
+d><td>$entry1[11]</td>
</tr>
<tr>
<td>$entry2[0]</td><td>$entry2[1]</td><td>$entry2[2]</td><td>$entry2[3
+]</td><td>$entry2[4]</td><td>$entry2[5]</td><td>$entry2[6]</td><td>$e
+ntry2[7]</td><td>$entry2[8]</td><td>$entry2[9]</td><td>$entry2[10]</t
+d><td>$entry2[11]</td>
</body></html>
</tr>
<tr>
<td>$entry3[0]</td><td>$entry3[1]</td><td>$entry3[2]</td><td>$entry3[3
+]</td><td>$entry3[4]</td><td>$entry3[5]</td><td>$entry3[6]</td><td>$e
+ntry3[7]</td><td>$entry3[8]</td><td>$entry3[9]</td><td>$entry3[10]</t
+d><td>$entry3[11]</td>
</tr>
<tr>
<td>$entry4[0]</td><td>$entry4[1]</td><td>$entry4[2]</td><td>$entry4[3
+]</td><td>$entry4[4]</td><td>$entry4[5]</td><td>$entry4[6]</td><td>$e
+ntry4[7]</td><td>$entry4[8]</td><td>$entry4[9]</td><td>$entry4[10]</t
+d><td>$entry4[11]</td>
</tr>
<tr>
<td>$entry5[0]</td><td>$entry5[1]</td><td>$entry5[2]</td><td>$entry5[3
+]</td><td>$entry5[4]</td><td>$entry5[5]</td><td>$entry5[6]</td><td>$e
+ntry5[7]</td><td>$entry5[8]</td><td>$entry5[9]</td><td>$entry5[10]</t
+d><td>$entry5[11]</td>
</tr>
</table>
<p style="text-align: center; font-size: large;"> <font color="red">**
+* PLEASE NOTE *** <br> If you are missing an entire entry please ensu
+re that you have provided the Offender's Full Name for each entry.<br
+>  PLEASE DO NOT REFRESH THIS PAGE, OR GO BACK AND RE-SUBMIT THIS INF
+ORMATION AS THIS WILL RESULT IN DUPLICATE ENTRIES IN THE DATABASE.<p>
</body>
</html>

HTML_SUCCESS
}
};

      [download]
      Again this is my first program so be nice :) Also, the goal of me cleaning up my code is to create a for loop for my data entry, AND data validation.
[reply]
[d/l]

        Honestly, I haven't been motivated to address this post since it'll take a bit of time and the issues I've already raised haven't been addressed.

        I'd also like to see the following replaced with CGI or CGI::Simple: 

        #Parse Data
read(STDIN, $buffer, $ENV{'CONTENT_LENGTH'});
@pairs = split(/&/, $buffer);
foreach $pair (@pairs) {
        ($name, $value) = split(/=/, $pair);
        $value =~ tr/+/ /;
        $value =~ s/%([a-fA-F0-9][a-fA-F0-9])/pack("C", hex($1))/eg;
        $value =~ s/^\s+//;
        $FORM{$name} = $value;
}

        [download]

        That means that instances of $FORM{XXX} become calls to param.

[reply]
[d/l]
[select]

In Section Seekers of Perl Wisdom