• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 254
  • Last Modified:

Review Script Please

The below script will be used to edit a news article in a flat file database.  The contents of the file are:

24|Kenny|EWPlanet Finale!|12:36 AM PST|3/04/2003|article|1
23|Kenny|EWPlanet Opens!|12:26 AM PST|3/04/2003|article|1
22|Kenny|Article Edited|12:36 AM PST|3/04/2003|The article that was edited|1
21|Kenny|EWPlanet Opens!|12:26 AM PST|3/04/2003|article|1
20|Kenny|EWPlanet Finale!|12:36 AM PST|3/04/2003|article|1
19|Kenny|EWPlanet Opens!|12:26 AM PST|3/04/2003|article|1
18|Kenny|EWPlanet Finale!|12:36 AM PST|3/04/2003|article|1
17|Kenny|EWPlanet Opens!|12:26 AM PST|3/04/2003|article|1

Last night the save portion of the script was working but it no longer is so I'm wondering what I might have did wrong.  If you can review the code, test it out on your website, and help me debug it... life would be great. ;)
_________________________________________________________

#!/usr/bin/perl

  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/"/'/g;
       $FORM{$name} = $value;
  }

#\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\

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

 $lnum = $FORM{'lnum'};
 $num = $FORM{'num'};

 $news_author = $FORM{'news_author'};
 $news_email = $FORM{'news_email'};
 $news_date = $FORM{'news_date'};
 $news_time = $FORM{'news_time'};
 $news_userid = $FORM{'news_userid'};
 $news_num = $FORM{'news_num'};
 $news_title = $FORM{'news_title'};
  $news_title =~ s/\cM//g;
  $news_title =~ s/\n\n/<p>/g;
  $news_title =~ s/\n/<br>/g;
  $news_title =~ s/&lt;/</g;
  $news_title =~ s/&gt;/>/g;
  $news_title =~ s/&quot;/"/g;
 $news_article = $FORM{'news_article'};
  $news_article =~ s/\cM//g;
  $news_article =~ s/\n\n/<p>/g;
  $news_article =~ s/\n/<br>/g;
  $news_article =~ s/&lt;/</g;
  $news_article =~ s/&gt;/>/g;
  $news_article =~ s/&quot;/"/g;

#\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\

 $qs = $FORM{'qs'};

   if ($qs eq "") { &choose; }
   if ($qs eq "choose") { &choose; }
   if ($qs eq "edit") { &edit; }
   if ($qs eq "save") { &save; }

#\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\

sub choose {

 open(ENEWS, "DB.txt");
  @enews = <ENEWS>;
 close (ENEWS);

 print qq~
  <html>
  <form method=POST action=edit.cgi>
  <input type=hidden name=qs value=edit>
 ~;

 foreach $line (@enews){
 chomp($line);
  ($lnum,$author,$title,$time,$date,$article,$userid)=split(/\|/,$line);
  if ($userid eq '1')
  {
 print qq~<div style="text-align: justify; font-family: verdana; font-size: 10pt;"><input type="radio" name="num" value="$lnum" class="radio"> $title on $date @ $time</font><br></div>\n~;
  }
 }
 print qq~
<div align=center>
<table border=0 cellspacing=0 cellpadding=0 class=fields_con>
 <tr>
  <td colspan=2><center><input type=submit value="edit news article"></center></td>
 </tr>
</table>
 </form>
 </html>
 ~;
}

#\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\

sub edit {

  print qq~
 <html>
 Edit Screen
 ~;

 open(ENEWS, "DB.txt");
  @enews = <ENEWS>;
 close (ENEWS);

 print qq~
  <html>
  <form method=POST action=edit.cgi>
  <input type=hidden name=qs value=save>
 ~;

 foreach $line (@enews){
 chomp($line);
  ($nnum,$author,$title,$time,$date,$article,$userid)=split(/\|/,$line);
  if ($nnum eq $num)
  {
print qq~
<div style="text-align: justify; font-family: verdana; font-size: 10pt;">
<input type=text name=news_num value="$nnum"><br>
<input type=text name=news_author value="$author"><br>
<input type=text name=news_title value="$title"><br>
<input type=text name=news_time value="$time"><br>
<input type=text name=news_date value="$date"><br>
<textarea COLS=53 ROWS=10 name=news_article>$article</textarea><br>
<input type=text name=news_userid value="$userid"><br>
</div>
~;
  }
 }
 print qq~
<div align=center>
<table border=0 cellspacing=0 cellpadding=0>
 <tr>
  <td colspan=2><center><input type=submit value="save news article"></center></td>
 </tr>
</table>
 </form>
 </html>
 ~;


}

#\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\

sub save {
print qq~
<html>\n
start script
<p>\n\n
$news_num<br>
$news_author<br>
$news_title<br>
$news_time<br>
$news_date<br>
$news_article<br>
$news_userid<br>
~;

$form_line = "$news_num";
$form_author = "$news_author";
$form_title = "$news_title";
$form_time = "$news_time";
$form_date = "$news_date";
$form_article = "$news_article";
$form_userid = "$news_userid";

open (DB,"<DB.txt") || die "cant open DB:$!";
@array = <DB>;
close (DB);

foreach (@array)
{
    chomp $_;
    my ($art_num, $art_author, $art_title, $art_time, $art_date, $art_article, $art_userid) = split (/\|/,$_);
    if ( $art_num eq $form_line && $form_userid eq $art_userid)
    {
        ### update details
        $art_title = $news_title;
        $art_article = $news_article;
    }
    $record = join('|', $art_num, $art_author, $art_title, $art_time, $art_date, $art_article, $art_userid );
    push @updates_file, $record . "\n";
   
}


open (DB,">DB.txt") || die "cant open DB:$!";
print DB @updates_file;
close (DB);

print qq~
$art_num, $art_author, $art_title, $art_time, $art_date, $art_article, $art_userid
<p>
end script\n\n

<form method=POST>
<input type=submit name=qs value=choose>
</form>

~;
}
0
KenHeckert
Asked:
KenHeckert
  • 3
  • 2
1 Solution
 
TintinCommented:
Lot's there, but I'll give you a few general pointers.

1.  Not using the standard CGI.pm module.  This will shorten your code significantly, and make it much more robust.
2.  Not checking the success of opening files.
3.  No file locking.
4.  Reading files needlessly into memory.
0
 
KenHeckertAuthor Commented:
I have no knownledge of the CGI.pm module.  I know that the files are opening, always have... that wasn't the problem.  Why would I need to lock the files?  I don't understand your comment about the memory part.

Keep in mind, I'm an amatuer at best when it comes to perl.

The problem has been fixed, it was:

if ( $art_num eq $form_line && $form_userid eq $art_userid)

That line needed to be:

if ( $form_line eq $art_num && $form_userid eq $art_userid)

Please answer my questions, hopefully this won't crash a system or something.
0
 
TintinCommented:
The CGI module is a standard module that has been shipped with the core Perl distribution for years now.  It is well developed and maintained.  Parsing CGI parameters is very difficult to do properly unless you have a very good in depth knowledge of the CGI specification.

To view the CGI documentation, type in

perldoc CGI

at your command prompt.

If you have ActiveState Perl installed, just use the HTML documentation that comes with it.

If you don't have Perl installed locally, go to http://perldoc.com and type CGI in the search field.

It is extremely easy to use.

To convert your broken parser, just replace it with

use CGI;
$q = new CGI;
%FORM = $q->Vars;

But please read the documentation to see all the other things it can do for you.

You should never, ever, make assumptions about files opening correctly.  It is all too easy for it to fail if the file is deleted, the permissions change, the disk gets corrupted etc etc.

Make sure you open files like:

open FILE, "/some/file.txt" or die "Can not open /some/file.txt because $!\n";

The $! variable is extremely useful as it contains the reason why the open failed, eg: "permission denied", "file not found" etc.

You should always use locking if there are going to be potentially multiple users accessing the script at the same time.  If user X and Y press the submit button at the same time, the data written back out is potentially not accurate if the second read occurs, before the first write has completed.

To read more on locking, see:

perldoc -q locking

My comment about reading files into memory is to alert you to the bad habit of people doing stuff like:

open DB,"DB.txt" or die "cant open DB:$!\n";
@array = <DB>;
close DB;

foreach (@array)
{
  print;
}

instead of:

open DB,"DB.txt" or die "cant open DB:$!\n";
while (<DB>) {
   print;
}

Apart from being a lot less lines of code, it will save you if you are dealing with very large files that are too large to fit into memory.



0
 
KenHeckertAuthor Commented:
Thanks TinTin, if I have more questions will you be available?
0
 
TintinCommented:
I check EE everyday
0

Featured Post

Free Tool: SSL Checker

Scans your site and returns information about your SSL implementation and certificate. Helpful for debugging and validating your SSL configuration.

One of a set of tools we are providing to everyone as a way of saying thank you for being a part of the community.

  • 3
  • 2
Tackle projects and never again get stuck behind a technical roadblock.
Join Now